feat: platform-address funding from asset-lock proofs#3671
Conversation
…lock/orchestration Extracts the submission-side machinery that's been identity-specific in `wallet/identity/network/registration.rs` into a new sibling module `wallet/asset_lock/orchestration.rs`. The three pieces lifted — `submit_with_cl_height_retry`, `resolve_funding_with_is_timeout_fallback`, and `out_point_from_proof` — were already funding-target-agnostic (generic over the submit closure / parameterized by `funding_type`) but lived inside `IdentityWallet`'s `impl` blocks. The platform-address funding flow needs the same three pieces verbatim, so consolidating them avoids ~150 lines of would-be duplication and locks the CL-retry policy + IS→CL fallback shape to one source of truth. `IdentityFunding` is renamed to `AssetLockFunding` in its new location (funding source is target-agnostic; the resolver's `funding_type` argument picks the BIP44 derivation family). A `pub use ... as IdentityFunding` alias stays at the old path so existing callers and the FFI surface keep compiling. `out_point_from_proof` is now a free `pub(crate) fn` rather than an associated function — it has no manager state dependency, and the manager's generic over `B: TransactionBroadcaster` would force the turbofish on every call site otherwise. The `submit_with_cl_height_retry_bumps_user_fee_and_surfaces_after_budget` test moved with its subject so the regression pin stays beside the code it protects. Pure mechanical move — no behavior change. 124/124 platform-wallet lib tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mAssetLock Mirrors the identity-create pattern from PR #3634: routes the outer asset-lock-proof signature on `AddressFundingFromAssetLockTransition` through an external `key_wallet::signer::Signer` rather than a raw `&[u8]` private key. The atomic derive + sign + zeroise sequence happens inside the signer's trust boundary — Rust never sees the asset-lock private key on this path. - Renamed the existing `try_from_asset_lock_with_signer` to `try_from_asset_lock_with_signer_and_private_key` for parity with the identity-create rename. Four downstream call sites (`rs-sdk/top_up_address`, two `strategy-tests`, drive-abci tests) updated in the same commit. - Added `try_from_asset_lock_with_signers<S, AS>` on the V0 inner + outer-enum dispatcher. `S: Signer<PlatformAddress>` signs each per-input `AddressWitness`; `AS: key_wallet::signer::Signer` signs the outer state-transition wrapper signature via `StateTransition::sign_with_core_signer`. - Both `signature` and `input_witnesses` carry `#[platform_signable(exclude_from_sig_hash)]`, so the signable bytes are computed once over the unsigned shape and used for both signing phases. Compiles workspace-clean; 90/90 address_funds tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the identity-side `broadcast_request_for_new_identity_with_signer` from PR #3634: routes address-funding asset-lock signing through an external `key_wallet::signer::Signer` instead of a raw `PrivateKey`, so Swift (and any other host holding keys outside Rust) can fund platform addresses without exposing the asset-lock private key across the FFI boundary. The new method threads `settings.user_fee_increase` straight through to the DPP `try_from_asset_lock_with_signers` builder. This is load-bearing for the upstream CL-height retry path in `platform-wallet::wallet::asset_lock::orchestration::submit_with_cl_height_retry`, which bumps `user_fee_increase` between attempts to change the ST's signable bytes — without this plumbing, retries would hash identically and Tenderdash's 24h invalid-tx cache would silently drop them. Existing `top_up` (raw private key) kept for callers that still hold the asset-lock key in Rust. Both impls share a new `broadcast_and_collect_address_infos` helper that owns the proof-shape verification + `AddressInfos` extraction so the two flows can't drift apart on the post-broadcast contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of `IdentityWallet::register_identity_with_funding` for the platform-address side. Drives the full build → broadcast → wait-IS-or-CL → submit-with-CL-retry → IS→CL-fallback → consume pipeline, with no asset-lock private key crossing the FFI boundary — the asset-lock signer's atomic derive + sign + zeroise sequence handles both Core-side key derivation (inside `AssetLockManager`) and the outer ST signature (`StateTransition::sign_with_core_signer`). Threaded `Arc<AssetLockManager<SpvBroadcaster>>` into `PlatformAddressWallet` so the new method can drive the same tracked locks every other sub-wallet sees. `PlatformAddressWallet::new` gains an `asset_locks` parameter; the only caller (`PlatformWallet::new`) clones the existing handle. The pre-existing raw-private-key `fund_from_asset_lock` is kept for callers that hold the asset-lock key in Rust, and now delegates its post-success balance-write bookkeeping to a shared `write_address_balances_changeset` helper that both paths use — no drift between the two flows on the changeset shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…signer New entry point that drives the wallet's orchestrated platform-address funding pipeline (build → IS-or-CL → submit → consume) using the same `(SignerHandle, MnemonicResolverHandle)` pair shape as the identity-side `platform_wallet_register_identity_with_funding_signer`. The asset-lock private key never crosses the FFI boundary as raw bytes — both Core-side derivation and the outer ST signature go through `MnemonicResolverCoreSigner`. Sister `platform_address_wallet_resume_fund_with_existing_asset_lock_signer` for the crash-recovery shape (resume from a tracked outpoint instead of building a fresh asset lock). The pre-existing raw-private-key `platform_address_wallet_fund_from_asset_lock` is kept; its hardcoded `Network::Mainnet` `PrivateKey` construction is now replaced with the wallet's own `network()`, fixing a latent bug that would produce a mismatched-network `PrivateKey` on testnet/devnet/regtest. ECDSA signing itself is network-agnostic so the bug never bit, but the value would have been wrong if ever re-exported. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Thin Swift wrappers over the new Rust FFI entry points. Each is a single Task.detached → withExtendedLifetime((signer, coreSigner)) → withUnsafeBufferPointer → FFI call → decode-changeset shape, with a synchronous pre-flight that mirrors the Rust-side invariant checks (non-empty recipients, exactly one nil-credits remainder, 20-byte hashes) so the user sees a fail-fast error before paying for the Task spin-up. The internal `MnemonicResolver()` is held by the calling actor through the FFI call via `withExtendedLifetime` — never `_ = signer`, which the optimizer can elide in -O builds and drop the resolver mid-call → use-after-free in the vtable callback. Same lifetime discipline as `ManagedPlatformWallet.registerIdentityWithFunding` / `resumeIdentityWithAssetLock` from PR #3634. Per the swift-sdk CLAUDE.md "thin marshaler" rule: no decisions in Swift. All orchestration (build asset-lock tx, wait IS/CL, submit ST, consume on success) lives in Rust's `PlatformAddressWallet::fund_addresses_with_funding`. Swift just marshals recipients + fee strategy + signer handles in and decodes the proof-attested balances out. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
End-to-end UI surface for the new fundFromCoreAssetLock flow. Presented as a sheet from `WalletDetailView` (third action button next to Send / Receive), the view drives the full pipeline from a single screen: Core BIP44 funding account picker (filters to standardTag = 0) → DIP-17 platform-payment account picker (accountType = 14) → unused-address picker (auto-selects lowest-index unused row) → amount-in-DASH input → Fund Address button Submit calls `addressWallet.fundFromCoreAssetLock(...)` against the selected wallet's managed handle. The Rust side owns every non-marshaling decision per the swift-sdk CLAUDE.md "thin marshaller" rule — Swift only collects user input, looks up the managed wallet via `walletManager.wallet(for:)`, and renders the proof-attested credit balance returned from the FFI. Single-recipient remainder pattern: the auto-picked unused address gets the entire asset-lock value (`credits = nil`); the on-chain fee is deducted from the same output via the `ReduceOutput(0)` fee strategy. Multi-recipient flows can be added later if needed; the SDK method already supports them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enables funding Platform (P2PKH) addresses directly from Core Layer 1 asset locks using external signers, eliminating private-key exposure over FFI. It extends DPP state transitions with explicit signer constructors, introduces shared orchestration for IS→CL fallback and CL-height retries, refactors identity funding to reuse the orchestration layer, and adds comprehensive Swift SDK and example-app UI for address-funding flows with progress tracking and resumption support. ChangesRust signer-based asset-lock funding
Swift SDK and example app signer-based funding UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive-abci/tests/strategy_tests/strategy.rs (1)
2602-2623:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale balance state when funding transition creation fails.
Line 2602 mutates
current_addresses_with_balancebefore the transition is built, and Line 2623 (.ok()?) silently drops builder/signing errors. That can leave a credited address without a corresponding transition and mask test regressions.Proposed fix
- current_addresses_with_balance.register_new_address_keep_only_highest( - address, - funded_amount, - self.max_addresses_to_choose_from_in_cache, - ); let mut outputs = BTreeMap::new(); outputs.insert(address.clone(), None); tracing::debug!(?outputs, "Preparing funding transition"); let funding_transition = AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key( asset_lock_proof, asset_lock_private_key.as_slice(), BTreeMap::new(), outputs, vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], signer, 0, platform_version, ) .await - .ok()?; + .expect("expected to create address funding from asset lock transition"); + + current_addresses_with_balance.register_new_address_keep_only_highest( + address, + funded_amount, + self.max_addresses_to_choose_from_in_cache, + ); Some(funding_transition)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/tests/strategy_tests/strategy.rs` around lines 2602 - 2623, The test mutates current_addresses_with_balance via current_addresses_with_balance.register_new_address_keep_only_highest before calling AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key, and then silently drops errors with .ok()?, leaving stale credited addresses if transition creation fails; change the flow so you only call register_new_address_keep_only_highest after the try_from_asset_lock_with_signer_and_private_key future returns Ok (or, alternatively, attempt the transition first and on Err do not mutate the cache or explicitly rollback the registration), i.e., move the call to current_addresses_with_balance.register_new_address_keep_only_highest (or perform a compensating removal) to occur after successful creation of funding_transition (the result of try_from_asset_lock_with_signer_and_private_key) so failures don’t leave inconsistent state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs`:
- Around line 221-240: In decode_funding_addresses, currently using
address_map.insert silently overwrites duplicates; change the logic to detect
duplicate recipients (e.g., check address_map.contains_key(&addr) or use the
BTreeMap::entry API) when iterating over FundingAddressEntryFFI entries and
return a PlatformWalletFFIResult::err (use
PlatformWalletFFIResultCode::ErrorInvalidParameter) with a message mentioning
the duplicate platform address (derived from entry.address) instead of
inserting, so duplicate funding recipients are rejected.
In
`@packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs`:
- Around line 250-254: The code currently swallows a successful on-chain/top-up
by returning PlatformAddressChangeSet::default() when
wm.get_wallet_info_mut(...) or
core_wallet.platform_payment_managed_account_at_index_mut(...) is missing, which
yields a silent empty local changeset; instead, when submit succeeded but the
wallet or account cannot be found for write-back, return an explicit error or
construct a changeset that reflects the confirmed top-up (do not use
PlatformAddressChangeSet::default()). Update the branches around
wm.get_wallet_info_mut(&self.wallet_id) and
platform_payment_managed_account_at_index_mut(platform_account_index) to either
(a) return Err(...) with context about the successful submit and missing
wallet/account so the caller can reconcile, or (b) build and return a
PlatformAddressChangeSet that includes the applied top-up amount and metadata
from the submit result; apply the same change to the similar check at the other
location (lines ~290-291).
- Around line 271-281: The code silently defaults address_index to 0 when no
matching address is found, which hides invariant breaks and can corrupt
PlatformAddressChangeSet; replace the unwrap_or(0) on the result of the find_map
over account.addresses.addresses (the block that calls
PlatformP2PKHAddress::from_address and compares to p2pkh) with a proper error
propagation: return an Err (or use expect with a clear message) when no matching
index is found, and update the surrounding function to propagate that error (use
? or map_err as appropriate) so missing address_index surfaces instead of
defaulting to the first address.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- Around line 460-465: The code hardcodes the fee-strategy discriminant by
constructing FeeStrategyStepFFI(step_type: 1, index: remainderIndex) which
couples Swift to Rust's enum layout; change this to use a symbol exported from
FFI (or a generated Swift constant) for the ReduceOutput variant instead of the
literal 1 (e.g., replace step_type: 1 with step_type:
FFI_FEE_STRATEGY_REDUCE_OUTPUT or the generated Swift enum/case provided by the
bindings), update all occurrences such as the FeeStrategyStepFFI creation near
ffiAddresses and the similar block at lines ~558-563 to reference that exported
constant/enum case, and if needed add the new constant in the Rust FFI layer so
the Swift wrapper consumes the canonical discriminant rather than mirroring it.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift`:
- Around line 275-290: coreAccountOptions currently includes accounts with zero
or insufficient confirmed balance and canSubmit does not validate the selected
account against parsedDuffs, allowing impossible submissions; update
coreAccountOptions (and the similar getter around lines 324-330) to filter
accounts by balanceConfirmed >= parsedDuffs or, alternatively, keep
coreAccountOptions as-is but modify canSubmit to check
selectedAccount.balanceConfirmed >= parsedDuffs before allowing submission.
Reference the CoreAccountOption mapping (accountIndex, balanceDuffs), the
coreAccountOptions computed property and the canSubmit logic, and ensure the UI
picker only shows spendable accounts or that canSubmit gates submission based on
balanceDuffs >= parsedDuffs.
---
Outside diff comments:
In `@packages/rs-drive-abci/tests/strategy_tests/strategy.rs`:
- Around line 2602-2623: The test mutates current_addresses_with_balance via
current_addresses_with_balance.register_new_address_keep_only_highest before
calling
AddressFundingFromAssetLockTransitionV0::try_from_asset_lock_with_signer_and_private_key,
and then silently drops errors with .ok()?, leaving stale credited addresses if
transition creation fails; change the flow so you only call
register_new_address_keep_only_highest after the
try_from_asset_lock_with_signer_and_private_key future returns Ok (or,
alternatively, attempt the transition first and on Err do not mutate the cache
or explicitly rollback the registration), i.e., move the call to
current_addresses_with_balance.register_new_address_keep_only_highest (or
perform a compensating removal) to occur after successful creation of
funding_transition (the result of
try_from_asset_lock_with_signer_and_private_key) so failures don’t leave
inconsistent state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0af12630-b1e9-426d-ac09-66abb82fbb24
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/mod.rspackages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/methods/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/v0/v0_methods.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rspackages/rs-drive-abci/tests/strategy_tests/strategy.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rspackages/rs-platform-wallet-ffi/src/platform_addresses/mod.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/wallet/asset_lock/mod.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/registration.rspackages/rs-platform-wallet/src/wallet/identity/types/funding.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rspackages/rs-platform-wallet/src/wallet/platform_addresses/mod.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-sdk/src/platform/transition/top_up_address.rspackages/strategy-tests/src/lib.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (1.26%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3671 +/- ##
============================================
- Coverage 87.84% 87.15% -0.70%
============================================
Files 2537 2600 +63
Lines 311315 318030 +6715
============================================
+ Hits 273472 277171 +3699
- Misses 37843 40859 +3016
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly mirrors the identity-side asset-lock funding pipeline (PR #3634) for platform addresses, with a thoughtful refactor lifting shared retry/resolver logic into asset_lock/orchestration. No blocking correctness or security issues identified after verification — the duplicate-recipient concern raised by codex as blocking matches existing patterns in parse_outputs and the legacy fund_from_asset_lock FFI, so it is downgraded to a suggestion. Main concerns are test coverage on the new orchestrator wrapper, a missing persister call (inconsistent with transfer.rs but consistent with the legacy raw-key funding path), and several minor contract/doc/UX mismatches.
🟡 4 suggestion(s) | 💬 6 nitpick(s)
10 additional finding(s)
suggestion: Duplicate funding recipients silently collapse in BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)
decode_funding_addresses inserts each FFI row into a BTreeMap without checking key collisions, so if Swift submits two rows for the same PlatformAddress, the second silently overwrites the first. Two consequences: (a) the funded output set differs from what the caller's [FundingRecipient] ordering and remainderIndex (ManagedPlatformAddressWallet.swift:460-465, 558-563) describe, and (b) a duplicate that flips has_balance can re-designate which entry is the None remainder, changing fee-strategy targeting after deduplication.
Note: this same silent-insert pattern exists in the sibling parse_outputs (platform_address_types.rs:189-204) used by the transfer path and in the legacy fund_from_asset_lock.rs:35-44 decode loop — codex's claim that 'the transfer API already rejects duplicate recipients' is inaccurate. So this is a pre-existing FFI pattern rather than a new regression, but the new funding API is a good place to start tightening it: rejecting duplicates here protects callers from a class of confusing protocol-side failures.
pub(super) unsafe fn decode_funding_addresses(
addresses: *const FundingAddressEntryFFI,
addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance { Some(entry.balance) } else { None };
if address_map.insert(addr, balance).is_some() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"duplicate platform address in funding recipients",
));
}
}
Ok(address_map)
}
suggestion: Funding success path doesn't push changeset through the persister
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 201)
After a successful submit, write_address_balances_changeset updates ManagedPlatformAccount in memory and returns a PlatformAddressChangeSet, but the orchestrator does not call self.persister.store(cs.clone().into()). The transfer path (transfer.rs:155-159) explicitly persists, with a long comment explaining that without persistence initialize_from_persisted on the next process start would seed stale balances and break auto_select_inputs.
The legacy fund_from_asset_lock (fund_from_asset_lock.rs:100-102) has the same gap, so this PR isn't introducing the divergence — it's preserving it on the new orchestrated path. Worth closing on both paths so the new user-facing fund-from-Core flow is crash-resumable without relying on the next BLAST sync to reconcile.
if !cs.addresses.is_empty() {
if let Err(e) = self.persister.store(cs.clone().into()) {
tracing::error!("Failed to persist platform-address funding changeset: {}", e);
}
}
Ok(cs)
suggestion: Swift FundingRecipient advertises addressType=1 (P2SH) but Rust FFI rejects it
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
FundingRecipient docs at :382 say 0 = P2PKH, 1 = P2SH, and both new entry points forward addressType unchanged into PlatformAddressFFI (:449, :551). On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress at packages/rs-platform-wallet-ffi/src/platform_address_types.rs:40-42 only accepts 0 and returns Unsupported address type for anything else. The Rust orchestrator also explicitly enforces P2PKH-only (fund_with_funding.rs:336-339).
Swift callers will only learn this after crossing the FFI boundary and getting a generic invalid-parameter error. The preflight is the right place to surface it.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: No direct tests for fund_addresses_with_funding orchestration branches
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
submit_with_cl_height_retry has an explicit regression-pinning test (orchestration.rs:476), and the underlying DPP and SDK primitives are exercised in their crates. But the new orchestrator itself — which composes pre-flight + funding resolution + retry submission + IS→CL rejection fallback + balance writeback + asset-lock consume — has no direct test.
The most fragile branch is the IS-rejection fallback at lines 173-197: it restarts submit_with_cl_height_retry from the original settings (settings is Copy, so the bumped value from the first call is discarded). That is intentional (the proof type changes, so the ST hash differs and Tenderdash's invalid-tx cache no longer applies), but a follow-up refactor switching to &mut settings would silently break it. The PR description also marks end-to-end testnet validation as still pending. A targeted #[tokio::test] (e.g. with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, would lock in the orchestration shape.
nitpick: Silent unwrap_or(0) for address_index in funding changeset writeback
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)
write_address_balances_changeset finds address_index by walking account.addresses.addresses and matching by PlatformP2PKHAddress, falling back to 0 on miss. Pre-flight uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool hasn't been hydrated for the given gap-limit slot, and the from_address(...).ok() silently swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry attributing the funded balance to address_index = 0 while every other field is correct — exactly the kind of silent off-by-one a SwiftData persister or UI will surface as 'all my funds landed on address #0'.
The same pattern exists in transfer.rs:119-129 (preserved, not introduced), but the orchestrated funding path has more chances for divergence because the address may have just been auto-picked from the unused pool. Prefer tracing::warn! on the fallback or skip the changeset entry when the index can't be resolved.
nitpick: Inline Duration::from_secs(300) should join the other named timeout constants
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 404)
The module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Promote to e.g. IS_LOCK_RESUME_TIMEOUT so a future tuning pass touches one site. Also note: fund_with_funding.rs:143 resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait, while the resolver here uses 300s — worth a comment if intentional or aligning if not.
nitpick: Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)
The comment on decode_funding_addresses claims it's 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' In fact, platform_addresses/fund_from_asset_lock.rs:35-44 still inlines its own decode loop with for entry in std::slice::from_raw_parts(addresses, addresses_count) { let addr = unwrap_result_or_return!(...); }. The two paths are not actually sharing decoding logic, so a future tightening (e.g. duplicate rejection, P2SH boundary check, error-message wording) on one will silently diverge from the other. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.
nitpick: Single DAPI 10506 response can force fee escalation across new funding path
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 217)
submit_with_cl_height_retry treats a single InvalidAssetLockProofCoreChainHeightError from the connected DAPI node as authoritative and bumps PutSettings::user_fee_increase before resubmitting. With fund_addresses_with_funding now wrapping Core-funded address top-ups in this helper, a malicious or desynchronized node can drive up to ~CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAY retries with escalating fees per address-funding attempt without holding the asset-lock key.
Not a new vector in this PR — the identity-funding path uses the same helper — but worth tracking as a bounded griefing surface across a new trust boundary. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.
nitpick: Recipient picker over-restricts to !isUsed && balance == 0
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 309)
recipientCandidates requires both !$0.isUsed and $0.balance == 0. The Rust-side pre-flight in validate_recipient_addresses only requires that the address belong to the supplied platform-payment account — it does not require the address to be unused or empty. Users who have previously funded or received to a platform address cannot pick it again from this screen even though the protocol accepts it.
If the intent is to bias the default toward fresh addresses without forbidding used ones, prefer sorting (used last) over filtering them out, or split the filter from the auto-pick policy in autoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why.
nitpick: decode_funding_addresses relies on caller's check_ptr! for null-safety
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)
The safety doc says addresses must be non-null. Inside, it unconditionally calls std::slice::from_raw_parts(addresses, addresses_count) — which is UB on a null data pointer even when count == 0 (the function requires a non-null aligned dangling-or-valid pointer). Today's two callers both do check_ptr!(addresses) first, so it's sound. But because the helper is pub(super), a future caller in the same module that forgets the prelude is one keystroke from UB.
Sibling helpers in the same crate (parse_outputs, parse_explicit_inputs) defensively check if ptr.is_null() && count > 0 { ... } internally, but even that pattern isn't quite right for the count == 0 case. Adding if addresses.is_null() { return Err(...); } unconditionally aligns this helper with the rest of the crate without changing today's behavior.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Drops the top-level "Fund L2" action button next to Send / Receive in favor of a small `plus.circle.fill` icon button at the trailing edge of the Platform Balance row in `BalanceCardView`. The button still opens the same `FundPlatformAddressView` sheet — only the entry point moved. Rationale: the action-button strip is reserved for Core (L1) operations; a third button there was visually heavy and conceptually mismatched. The "+" lives directly next to the balance it tops up, which reads as the natural affordance. Implementation: - `WalletBalanceRow` gains an optional `TrailingAction` (system image + accessibility label + closure). The struct nests inside the row so call sites name it as `WalletBalanceRow.TrailingAction`. - `BalanceCardView` takes an optional `onFundPlatform: (() -> Void)?` callback and wires it onto the Platform row only. Read-only callers pass `nil` and the affordance disappears entirely. - `WalletDetailView` passes its `showFundPlatformAddress = true` closure through. The sheet presentation stays where it was. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…undPlatformAddressView Two bugs caught during testnet manual testing: 1. **Duplicate "Account #0" rows.** The Core-funding-account picker filtered by `standardTag == 0` alone, which meant every PlatformPayment / CoinJoin / Identity account also matched (they all leave `standardTag` at its `0` default — meaningless for non-Standard variants). Result: the picker showed one "Account #0" row per AccountType, all looking identical, and SwiftUI logged `ForEach<…>: the ID 0 occurs multiple times` warnings. Fix: the compound predicate is `typeTag == 0 && standardTag == 0` — BIP44 Standard only. 2. **Balance always read as zero.** The picker read `PersistentAccount.balanceConfirmed`, which is populated by the persister callback and lags the live Rust state. A freshly-synced wallet with spendable Core funds would still show zero here. Fix: route through `walletManager.accountBalances(for:)` — same source `BalanceCardView` already uses for the Core Balance row, so the user sees a consistent number. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at ff31d50. The new delta is a single SwiftUI commit (Fund L2 entry point relocated to an inline '+' affordance on the Platform Balance row) — verified clean, no new findings. All 10 prior findings from c547c4d were re-verified against current source and remain STILL VALID; none of the underlying Rust / FFI / Swift orchestration / picker code was touched. Note: the prior #8 (DAPI 10506 fee-escalation griefing) has gained explicit trust-model documentation at orchestration.rs:217-234 since the prior review — carried forward as a documented bounded risk rather than blocking. Carrying forward 10 findings within the 10-finding budget.
🟡 4 suggestion(s) | 💬 6 nitpick(s)
10 additional finding(s) omitted (not in diff).
Findings not attached inline (unchanged lines / prior carried-forward findings)
-
**[suggestion] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:594 — Swift FundingRecipient advertises addressType=1 (P2SH) but Rust FFI rejects it
Verified at ff31d50: the Swift
FundingRecipientdoc states0 = P2PKH, 1 = P2SH, and bothfundFromCoreAssetLockandresumeFundFromAssetLockforwardaddressTypeunchanged intoPlatformAddressFFI. On the Rust side,TryFrom<PlatformAddressFFI> for PlatformAddressonly accepts 0 and returns 'Unsupported address type' for anything else; the orchestrator additionally enforces P2PKH-only.fundFromCoreAssetLockPreflight(594-613) checkshash.count == 20but does NOT checkaddressType == 0, so callers learn the constraint only after crossing the FFI boundary and getting a generic invalid-parameter error. The preflight is the right place to surface it — alternatively, narrow the Swift API surface (e.g. drop the byte and assume P2PKH) until P2SH is actually implemented on the Rust side. -
**[suggestion] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:221 — Duplicate funding recipients silently collapse in BTreeMap decode
Verified at ff31d50 (lines 221-241 unchanged from c547c4d):
decode_funding_addressesinserts each FFI row into aBTreeMapwithout checking key collisions. If Swift submits two rows for the samePlatformAddress, the second silently overwrites the first. Two consequences: (a) the funded output set diverges from the caller's[FundingRecipient]ordering andremainderIndexcomputation (Swift indexes the pre-collapse array at ManagedPlatformAddressWallet.swift:460-465, 558-563), and (b) a duplicate that flipshas_balancecan re-designate which entry is theNoneremainder, changing fee-strategy targeting.BTreeMap::insert's return value already detects the collision at zero extra cost. -
**[suggestion] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:205 — Funding success path doesn't push changeset through the persister
Verified at ff31d50 (lines 205-223 unchanged from c547c4d): after a successful submit,
write_address_balances_changesetmutatesManagedPlatformAccountin memory and returns aPlatformAddressChangeSet, but the orchestrator never callsself.persister.store(cs.clone().into()). The transfer path explicitly persists (with a comment about howinitialize_from_persistedwould otherwise seed stale balances and breakauto_select_inputson next process start). The legacyfund_from_asset_lockhas the same gap, so this PR preserves rather than introduces the divergence — but closing it on the new path makes the user-facing fund-from-Core flow crash-resumable without relying on the next BLAST sync to reconcile. Alternatively, change the return type so the caller is forced to acknowledge the not-yet-persisted state. -
**[suggestion] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:88 — No direct tests for fund_addresses_with_funding orchestration branches
Verified at ff31d50:
submit_with_cl_height_retryhas an explicit regression-pinning test and the underlying DPP/SDK primitives are exercised elsewhere, but the new orchestrator — composing pre-flight, funding resolution, retry submission, IS→CL rejection fallback, balance writeback, and asset-lock consume — has no direct test. The most fragile branch is the IS-rejection fallback at lines 173-197: it restartssubmit_with_cl_height_retryfrom the originalsettings(settings isCopy, so the bumped value from the first call is discarded). That is intentional (the proof type changes, so the ST hash differs and Tenderdash's invalid-tx cache no longer applies), but a follow-up refactor switching to&mut settingswould silently break it. A targeted#[tokio::test](withstart_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, would lock in the orchestration shape. PR description marks end-to-end testnet validation as still pending. -
**[nitpick] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:210 — Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate
Verified at ff31d50: the comment on
decode_funding_addressesclaims it's 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' In fact,platform_addresses/fund_from_asset_lock.rs:35-44still inlines its own decode loop. The two paths are not actually sharing decoding logic, so a future tightening (duplicate rejection, P2SH boundary check, error-message wording) on one will silently diverge from the other. Either route the legacy FFI throughdecode_funding_addressesto make the comment true, or correct the comment. -
**[nitpick] packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:402 — Inline Duration::from_secs(300) should join the other named timeout constants
Verified at ff31d50: the module declares
CL_FALLBACK_TIMEOUT(180s),CL_HEIGHT_RETRY_DELAY(15s), andCL_HEIGHT_RETRY_BUDGET(210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in theFromExistingAssetLockbranch at line 404. Promote to e.g.IS_LOCK_RESUME_TIMEOUT. Also:fund_with_funding.rs:143resumes withCL_FALLBACK_TIMEOUT(180s) for the same asset-lock manager wait, while the resolver here uses 300s — worth a comment if intentional or aligning if not. -
**[nitpick] packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:271 — Silent unwrap_or(0) for address_index in funding changeset writeback
Verified at ff31d50:
write_address_balances_changesetresolvesaddress_indexby walkingaccount.addresses.addressesand matching byPlatformP2PKHAddress, falling back to0withunwrap_or(0)on miss. Pre-flight usesaccount.contains_platform_address(&p2pkh)— a different collection — so the two views can disagree if the address pool hasn't been hydrated for the given gap-limit slot, andfrom_address(...).ok()silently swallows parse mismatches. The downstream effect is aPlatformAddressBalanceEntryattributing the funded balance toaddress_index = 0while every other field is correct — exactly the kind of silent off-by-one a SwiftData persister will surface as 'all my funds landed on address #0'. The same pattern exists intransfer.rs:119-129(preserved, not introduced). Prefertracing::warn!on the fallback, or skip the changeset entry when the index can't be resolved. -
**[nitpick] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift:309 — Recipient picker over-restricts to !isUsed && balance == 0
Verified at ff31d50 (line 312 unchanged — the latest delta refactored only WalletDetailView, not FundPlatformAddressView):
recipientCandidatesrequires both!$0.isUsedand$0.balance == 0. The Rust-side pre-flight invalidate_recipient_addressesonly requires that the address belong to the supplied platform-payment account — it does not require the address to be unused or empty. Users who have previously funded or received to a platform address cannot pick it again from this screen even though the protocol accepts it. If the intent is to bias the default toward fresh addresses, prefer sorting (used last) over filtering, or split the filter from the auto-pick policy inautoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why. -
**[nitpick] packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs:218 — decode_funding_addresses relies on caller's check_ptr! for null-safety
Verified at ff31d50: the
// Safetydoc requires non-nulladdresses. Inside, it unconditionally callsstd::slice::from_raw_parts(addresses, addresses_count)— which is UB on a nulldatapointer even whencount == 0per the slice contract. Today's two callers both docheck_ptr!(addresses)first, so it's sound. But the helper ispub(super), and a future intra-module caller that forgets the prelude — or fast-pathsaddresses_count == 0on the hunch that 'an empty array can't matter' — would silently introduce UB at an FFI trust boundary consuming attacker-controlled pointers. Sibling helpers (parse_outputs,parse_explicit_inputs) defensively null-check internally; aligning here costs nothing and removes a sharp edge from the boundary. -
**[nitpick] packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs:217 — Single DAPI 10506 response can force fee escalation across new funding path
Verified at ff31d50:
submit_with_cl_height_retrytreats a singleInvalidAssetLockProofCoreChainHeightErrorfrom the connected DAPI node as authoritative and bumpsPutSettings::user_fee_increasebefore resubmitting. Withfund_addresses_with_fundingnow wrapping Core-funded address top-ups in this helper, a malicious or desynchronized node can drive up to ~CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAYretries with escalating fees per address-funding attempt without holding the asset-lock key. The trust model is now explicitly documented at orchestration.rs:217-234 (bounded budget, attack-unprofitability reasoning), so this is documented technical debt rather than an undisclosed defect — carrying forward for visibility. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental re-review at 9889023. The single new commit since ff31d50 is SwiftUI-only (FundPlatformAddressView: BIP44 Core-account picker filter + live FFI balance) and is verified clean. All substantive findings from the prior cumulative review at ff31d50 were re-checked against current source at 9889023 and remain valid; the new commit does not touch the Rust orchestrator, FFI decoder, Swift preflight/marshaling, recipient-picker filter, or test surface. Two reviewers independently converged on the same 8 underlying issues; carrying forward 10 findings within budget.
🟡 4 suggestion(s) | 💬 6 nitpick(s)
10 additional finding(s)
suggestion: Swift preflight accepts `addressType` values that Rust then rejects across the FFI boundary
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 606)
Verified at 9889023: fundFromCoreAssetLockPreflight (594-613) validates hash.count == 20 but does not validate addressType. The public Swift FundingRecipient documents 0 = P2PKH, 1 = P2SH, and both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress only accepts 0 (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43) and the orchestrator additionally enforces P2PKH-only. A caller building a P2SH FundingRecipient therefore passes Swift preflight, crosses the FFI boundary, and surfaces only a generic invalid-parameter error from Rust. Surface this constraint in the preflight (or narrow the Swift API surface to drop the byte until P2SH is implemented on the Rust side).
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Duplicate funding recipients silently collapse in BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)
Verified at 9889023 (line 238): decode_funding_addresses calls address_map.insert(addr, balance) without inspecting the return value, so two FFI rows for the same PlatformAddress silently overwrite each other. Two concrete consequences across the boundary: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller passed in — which is significant because Swift derives the remainder index from that pre-collapse array (ManagedPlatformAddressWallet.swift:460-465, 558-563); and (b) if the duplicate row flips has_balance, the lone None remainder target can shift, changing fee-strategy attribution silently after dedup. BTreeMap::insert's return already exposes the collision at zero cost — reject explicitly at the FFI boundary.
pub(super) unsafe fn decode_funding_addresses(
addresses: *const FundingAddressEntryFFI,
addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
if addresses.is_null() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"addresses pointer is null",
));
}
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance { Some(entry.balance) } else { None };
if address_map.insert(addr, balance).is_some() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"duplicate platform address in funding recipients",
));
}
}
Ok(address_map)
}
suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)
Verified at 9889023 (lines 205-223): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists, with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. The legacy fund_from_asset_lock has the same gap, so this is preserved rather than introduced — but the new Core-funded user-facing path stays crash-fragile until a sync reconciles. Note the relevance to the new delta in this PR: the Swift app now reads Core balances live from accountBalances(for:) instead of stale SwiftData precisely to dodge the same lag — the Platform side has an exact mirror here.
let cs = self
.write_address_balances_changeset(platform_account_index, &address_infos)
.await;
if !cs.addresses.is_empty() {
if let Err(e) = self.persister.store(cs.clone().into()) {
tracing::error!("Failed to persist platform-address funding changeset: {}", e);
}
}
if let Some(out_point) = tracked_out_point {
// Cleanup failure can only mean WalletNotFound (the wallet
// handle that just funded the addresses vanished). Surface
// as a warn — Platform DID accept the top-up, so
// propagating the error to the caller would be misleading.
if let Err(e) = self.asset_locks.consume_asset_lock(&out_point).await {
tracing::warn!(
outpoint = %out_point,
error = %e,
"consume_asset_lock failed after successful Platform submit"
);
}
}
Ok(cs)
suggestion: No direct tests for fund_addresses_with_funding orchestration branches
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
Verified at 9889023: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value (settings is Copy, so the bumped value from the first call is discarded) because the proof type changes and the ST hash differs, so Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.
nitpick: Inline `Duration::from_secs(300)` should join the other named timeout constants
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 402)
Verified at 9889023 (line 404 confirmed): the module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Additionally, fund_with_funding.rs resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait while the resolver here uses 300s; if intentional, add a comment, otherwise align. The mismatch matters because both timeouts gate when the FFI top_up call returns to Swift. Promote to e.g. IS_LOCK_RESUME_TIMEOUT.
nitpick: Doc claim of "shared with the legacy raw-private-key FFI" is inaccurate
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)
Verified at 9889023 (lines 214-216): the doc on decode_funding_addresses states it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' That is not true — platform_addresses/fund_from_asset_lock.rs keeps its own inline for entry in from_raw_parts(...) decode loop. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.
nitpick: Silent `unwrap_or(0)` fabricates `address_index = 0` when recipient lookup fails
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)
Verified at 9889023 (lines 271-281): write_address_balances_changeset resolves address_index by walking account.addresses.addresses, parsing each stored address, matching by PlatformP2PKHAddress, and falling back to 0 via unwrap_or(0) on miss. Preflight membership uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool is not hydrated for the relevant gap-limit slot, and the from_address(...).ok() swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry with correct hash/balance but wrong address_index, which a SwiftData persister will surface as 'all my funds landed on address #0'. Same pattern exists in transfer.rs:119-129 (preserved, not introduced). Either tracing::warn! and skip the entry, or surface the invariant violation explicitly.
nitpick: Recipient picker over-restricts to `!isUsed && balance == 0`
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 317)
Verified at 9889023 (line 320): recipientCandidates requires !$0.isUsed && $0.balance == 0. The Rust-side preflight in validate_recipient_addresses only requires the address to belong to the supplied platform-payment account — it does not require unused or zero-balance. As a result, users cannot pick previously-funded or previously-received-on platform addresses from this screen even though the protocol and FFI accept them. If the intent is to bias the default toward fresh addresses, sort (used last) rather than filter, or split the filter from the auto-pick policy in autoSelectRecipient. If the intent is genuinely to forbid them, the UI should explain why.
nitpick: `decode_funding_addresses` relies on caller's `check_ptr!` for null-safety at an FFI trust boundary
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)
Verified at 9889023: the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses), so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! (or fast-paths addresses_count == 0 on the hunch that 'empty can't matter') would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate (parse_outputs, parse_explicit_inputs) defensively null-check internally; aligning here costs nothing.
pub(super) unsafe fn decode_funding_addresses(
addresses: *const FundingAddressEntryFFI,
addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
if addresses.is_null() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"addresses pointer is null",
));
}
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance { Some(entry.balance) } else { None };
address_map.insert(addr, balance);
}
Ok(address_map)
}
nitpick: Single DAPI 10506 response authoritatively bumps fees across new platform-address funding path
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 217)
Verified at 9889023: submit_with_cl_height_retry treats the first InvalidAssetLockProofCoreChainHeightError from the connected DAPI node as authoritative and bumps PutSettings::user_fee_increase before resubmitting, with up to roughly CL_HEIGHT_RETRY_BUDGET / CL_HEIGHT_RETRY_DELAY (~14) escalating retries. This PR extends the helper to wrap fund_addresses_with_funding, so a malicious or desynchronized DAPI node can now drive the same bounded fee-escalation grief on Core-funded platform-address top-ups in addition to identity registration. Attacker needs neither the asset-lock key nor Platform consensus control — just to be the queried endpoint. The trust model is explicitly documented in source (orchestration.rs:217-234) as accepted technical debt with a bounded-griefing/attack-unprofitability rationale, so this is carried forward for visibility rather than as an undisclosed defect. A future hardening pass should consider second-source confirmation or node rotation before accepting a fee bump.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Parity with the identity-side `RegistrationProgressView` for the
address-funding flow. Adds an `AddressFundingController` +
`AddressFundingCoordinator` + `AddressFundingProgressView` trio
mirroring the identity-side shape, and wires `FundPlatformAddressView`
to swap its form body for the 4-step progress section once Submit
fires.
Step mapping (driven by `PersistentAssetLock.statusRaw` + elapsed time
since `updatedAt`, same heuristic the identity side uses):
1. Building asset-lock transaction — statusRaw == 0
2. Broadcasting — statusRaw == 1, <2s since update
3. Waiting for InstantSend proof — statusRaw == 1, 2s..300s
(rendered as "Waiting for ChainLock proof" past the 300s IS
timeout to communicate that the wallet has fallen back to CL
finality — the step count stays at 4)
4. Funding platform address — statusRaw ∈ {2, 3}
Single-flight gate at the coordinator level (`(walletId,
platformAccountIndex, recipientHash)`) prevents a double-tap on
Submit from racing two FFI calls for the same asset lock.
Controllers survive view dismissal via the coordinator's `@Published`
map; the (forthcoming) "Pending Platform Funding" row on the wallet
detail screen will surface them after this commit.
Inline terminal section ("Address funded" / "Funding failed") embedded
directly in `FundPlatformAddressView`'s form so the user sees the
result without a separate navigation push. Standalone
`AddressFundingProgressView` is also exposed for the resumable-funding
surface.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift`:
- Around line 255-258: The footerText currently instructs "Tap Dismiss..." but
AddressFundingProgressView does not provide a dismiss action; update the view
(the block that renders the footer around where footerText is used, currently
the code section rendering the footer UI) to add a Button labeled "Dismiss" that
calls the same coordinator dismissal used in the inline funding view (call
coordinator.dismiss() or the same coordinator method used there). Keep
footerText as-is for the message, but render the Button when isFailed is true
and wire it to coordinator.dismiss() so tapping Dismiss clears the entry exactly
like the inline funding view.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72b743d3-fb27-4c90-85e4-4e95626f17ca
📒 Files selected for processing (6)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressFundingController.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressFundingCoordinator.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/PlatformWalletManager+AddressFundingCoordinator.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift
…ings
Adds a "Pending Platform Funding" card to `WalletDetailView`,
between the action buttons and the Transactions section. Two row
sources merge into the card:
1. In-flight `AddressFundingController`s from the coordinator's
`@Published controllers` map — the live submit-still-running
case. Tap drills into `AddressFundingProgressView`.
2. Orphaned `PersistentAssetLock` rows with
`fundingTypeRaw == AssetLockAddressTopUp` (4) and
`statusRaw ∈ [1, 3]` — crash-recovery case where the user
killed the app between asset-lock broadcast and ST
submission. Tap → "Resume" opens `FundPlatformAddressView`
in resume mode pre-seeded with the lock's outpoint.
`FundPlatformAddressView` gains an optional `resumeFromLock:
PersistentAssetLock?` parameter. When set:
- The form hides the Core-funding-account picker and amount
field (both were decided at original build time and live in
the persisted lock row).
- Submit routes to `resumeFundFromAssetLock` instead of
`fundFromCoreAssetLock`. The outpoint is parsed from the
persisted `outPointHex` (canonical `<txid display hex>:<vout>`
shape) and reversed back to wire order for the FFI.
- Submit-button label flips to "Resume Funding".
The orphan-lock anti-join machinery is wired with an empty
controller-side outpoint set today (the controller doesn't yet
expose its lock's outpoint). The SwiftData `statusRaw` upper bound
(<=3, excludes Consumed) already prevents stale rows from
surfacing once the ST has landed; the de-dupe set lives for a
future tweak where the controller can surface its outpoint after
the broadcast step.
The card collapses to nothing when neither source has rows, so a
freshly-synced wallet with nothing in flight doesn't see an
empty header.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rows Adds two optional fields to `PersistentAssetLock` (`recipientPlatformAddressHash: Data?` and `recipientPlatformAddressType: UInt8?`) populated by Swift after a successful `fundFromCoreAssetLock` call. Rust doesn't know the recipient — it's chosen at ST-submit time, not at asset-lock build time — so the back-fill is Swift-side. Back-fill mechanism: on `.completed` phase, `FundPlatformAddressView` fetches the newest matching consumed lock (`walletId, fundingTypeRaw==4, statusRaw==4, recipientHash==nil`) and stamps the recipient hash + type byte. Defaults to nil so SwiftData lightweight migration is safe; pre-this-commit rows render as "Recipient — (pre-this-commit row)" in the storage explorer rather than collapsing the section. `AssetLockStorageDetailView` gains a "Recipient Platform Address" section for `fundingTypeRaw == 4` rows. When the hash is set: - Hex hash - Address type label (P2PKH / P2SH) - DIP-0018 bech32m encoding (e.g. `tdash1k…`) Bech32m encoding is implemented inline as private file-scope helpers (convertBits + bech32mEncode + bech32mPolymod + bech32mHRPExpand + bech32mCreateChecksum) so the explorer doesn't take a new dependency. Mirrors the Rust-side `PlatformAddress::to_bech32m_string` byte-for-byte. The HRP defaults to testnet (`tdash`) — the common case in this example app; mainnet wallets can wire through later if needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
All major findings re-verified at 9efcf6e. No blocking issues. The SwiftExampleApp-only delta since 9889023 introduces one new concrete logic gap (AddressFundingProgressSection's @query has no per-recipient scope, so concurrent address-fundings on one wallet can alias onto the same progress row), plus a coordinator/sheet-lifetime gap that undercuts the stated goal of surviving sheet dismissal. The 10 prior Rust/FFI/Swift-SDK findings all reproduce at the same line ranges. Two additional prior nitpicks (recipient picker UX divergence; DAPI 10506 fee-escalation surface) are retained below as carried-forward prior findings outside the 10 expanded finding budget.
🟡 6 suggestion(s) | 💬 4 nitpick(s)
10 expanded finding(s)
suggestion: Swift preflight accepts `FundingRecipient.addressType` values that Rust then rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 9efcf6e (lines 594-613): fundFromCoreAssetLockPreflight checks recipients.isEmpty, the single-nil-remainder invariant, and hash.count == 20, but never inspects addressType. Both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43) only accepts 0, and the orchestrator additionally enforces P2PKH-only. A caller building a P2SH FundingRecipient therefore passes the synchronous preflight, spins up the Task, marshals the handle, and surfaces only a generic invalid-parameter error from Rust — defeating the entire point of the preflight, which the doc says exists to deliver a synchronous error before paying for the Task detach + handle marshaling. Either validate addressType == 0 in the preflight, or drop the byte from the public Swift surface until P2SH is actually wired up Rust-side.
suggestion: Duplicate funding recipients silently collapse in the FFI BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)
Verified at 9efcf6e (line 238): decode_funding_addresses calls address_map.insert(addr, balance) and ignores the return value, so two FFI rows for the same PlatformAddress silently overwrite each other at the C ABI boundary. Two concrete consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller built — Swift derives the remainder index from that pre-collapse array, so after dedup the Rust side may see a different recipient set and a different None remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder target can shift, redirecting fees and remainder credits silently. BTreeMap::insert's return value already exposes the collision at zero cost — reject explicitly at the boundary so the input shape the caller passed is the input shape the orchestrator executes.
suggestion: Progress section mis-attributes asset-lock state when two address-fundings run concurrently on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 51)
New at 9efcf6e. AddressFundingProgressSection.init constructs _activeLocks with a #Predicate scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc. The body then unconditionally drives currentStep, broadcastSubStep, and isInChainLockFallback from activeLocks.first (lines 109-127, 145-149, 154-163). AddressFundingController is keyed (walletId, platformAccountIndex, recipientHash) precisely to allow many concurrent fundings on one wallet (per its own doc), but PersistentAssetLock carries no recipient-hash linkage — only outPointHex, walletId, fundingTypeRaw, statusRaw, updatedAt. So when two AssetLockAddressTopUp flows are in flight, both progress views observe the same activeLocks query and both pick whichever lock most recently transitioned updatedAt. The identity-side analog disambiguates via identityIndexRaw; this view has no equivalent. Concrete consequence: Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast, even though A is still building its asset-lock tx. The controller's per-attempt .phase keeps .completed/.failed correct, but step count and CL-fallback footer can mis-attribute. Single-funding usage is unaffected; close this before the planned 'Pending Platform Funding' surface ships. Options: (a) return the asset-lock outpoint from the FFI on creation so the controller can scope its own @Query, or (b) add a recipient-hash field to PersistentAssetLock populated when the orchestrator picks the credit-output destination.
suggestion: In-flight funding is not restorable after sheet dismissal despite the new coordinator
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 59)
Verified at 9efcf6e: activeController is held only as @State local to this view (line 64) and never rehydrated from walletManager.addressFundingCoordinator on appear. The sheet is still presented via a plain .sheet from WalletDetailView, and .onAppear(perform: autoSelectDefaults) only resets defaults. So the user can dismiss the sheet interactively (Cancel is disabled while .inFlight, but swipe-down/interactive dismissal on iOS is not), and on re-presentation the view starts from activeController == nil and renders the blank form — the only way back to the original controller is to manually recreate the same slot and re-submit. That directly undercuts the in-source comment (lines 59-64) that lifetime ownership lives on the coordinator so 'view dismissal mid-flight doesn't lose the work'. Either rehydrate from the coordinator in .onAppear keyed by (walletId, platformAccountIndex, recipientHash), or block interactive dismissal while in-flight.
suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)
Verified at 9efcf6e (lines 205-223): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local wallet doesn't know — affecting auto-input selection and any UI reading balances from the persisted SwiftData store. The legacy fund_from_asset_lock has the same gap (preserved, not introduced). Notable: the new AddressFundingCoordinator keeps the controller alive across view dismissal precisely so the success transition is observable — that's negated for a process restart while the persister gap remains.
suggestion: No direct tests for fund_addresses_with_funding orchestration branches
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
Verified at 9efcf6e: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value — because PutSettings is Copy, the bumped value from the first call is discarded, which is required because the proof type changes, the ST hash differs, and Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that retry-cache-bypass invariant. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, and (c) consume-on-success would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.
nitpick: Doc claim of "shared with the legacy raw-private-key FFI" is inaccurate
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)
Verified at 9efcf6e (lines 214-216): the doc on decode_funding_addresses states it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' Confirmed false — platform_addresses/fund_from_asset_lock.rs:35-44 still has its own inline for entry in from_raw_parts(...) decode loop. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge across the boundary. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.
nitpick: Inline `Duration::from_secs(300)` should join the named timeout constants
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (line 402)
Verified at 9efcf6e (line 404): the module declares CL_FALLBACK_TIMEOUT (180s), CL_HEIGHT_RETRY_DELAY (15s), and CL_HEIGHT_RETRY_BUDGET (210s) as named policy knobs, and the module docs describe the IS-lock resume wait (300s) as policy too — but it's inlined as a bare literal in the FromExistingAssetLock branch. Separately, fund_with_funding.rs resumes with CL_FALLBACK_TIMEOUT (180s) for the same asset-lock manager wait while the resolver here uses 300s; if intentional, document why, otherwise align. Both timeouts gate when the FFI top_up call returns to Swift — and the new AddressFundingProgressSection.instantLockTimeout = 300.0 is yet another mirror that will drift independently from the Rust side. Promote to e.g. IS_LOCK_RESUME_TIMEOUT and reconcile the three sites.
nitpick: `decode_funding_addresses` relies on caller's `check_ptr!` for null-safety at an FFI trust boundary
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)
Verified at 9efcf6e: the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! — or fast-paths addresses_count == 0 on the hunch that 'empty can't matter' — would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate defensively null-check internally; aligning here costs nothing and removes a sharp edge from a security boundary.
nitpick: Silent `unwrap_or(0)` fabricates `address_index = 0` when recipient lookup fails
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 271)
Verified at 9efcf6e (lines 271-281): write_address_balances_changeset resolves address_index by walking account.addresses.addresses, parsing each stored address via PlatformP2PKHAddress::from_address(...).ok(), matching by PlatformP2PKHAddress, and falling back to 0 via unwrap_or(0) on miss. Preflight membership uses account.contains_platform_address(&p2pkh) — a different collection — so the two views can disagree if the address pool is not hydrated for the relevant gap-limit slot, and the .ok() swallows parse mismatches. The downstream effect is a PlatformAddressBalanceEntry with correct hash/balance but wrong address_index, which a SwiftData persister will surface as 'all my funds landed on address #0'. If a downstream operation ever derives signing keys or HD paths from address_index without re-matching the hash, the silent fabrication becomes a key-derivation mismatch. Same pattern exists in transfer.rs:119-129 (preserved, not introduced). Either tracing::warn! and skip the entry, or surface the invariant violation explicitly.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Carried-forward prior findings retained from 9889023
These two 9889023 findings were also re-validated as still applicable at 9efcf6e2; I am keeping them in the review body explicitly so the cumulative review does not erase prior unresolved findings.
nitpick: Recipient picker over-restricts to `!isUsed && balance == 0`
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 320)
Still valid at 9efcf6e2: recipientCandidates continues to filter candidate platform addresses with !$0.isUsed && $0.balance == 0, while the Rust-side validate_recipient_addresses path only requires that the address belong to the supplied platform-payment account. That keeps the example UI narrower than the wallet orchestration actually supports, so users cannot pick some valid addresses through the demo flow.
nitpick: Single DAPI 10506 response authoritatively bumps fees across the new funding path
packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs (lines 217-297)
Still valid at 9efcf6e2: submit_with_cl_height_retry still treats the first InvalidAssetLockProofCoreChainHeightError response from the connected DAPI node as authoritative enough to increase PutSettings::user_fee_increase before resubmitting. This remains an accepted technical-debt/security-surface nitpick rather than a blocker, but it is still part of the cumulative finding set for this head.
The previous shape collapsed "Waiting for InstantSend proof" and
"Waiting for ChainLock proof" into a single dynamic step 3 that
renamed itself once the IS deadline passed. Two practical problems:
- The CL fallback was visually invisible. A user watching the
progress screen had no way to tell whether the lock resolved via
IS or CL — both lanes produced the same "step 3 done ✓".
- The total step count was 4, asymmetric with the identity-side
progress view (5 steps, same shape, IS and CL on their own rows).
The asymmetry was confusing for users who had already learned
the identity flow.
Restored the 5-step shape:
1. Building asset-lock transaction
2. Broadcasting
3. Waiting for InstantSend proof (skipped if CL resolved first)
4. Waiting for ChainLock proof (skipped if IS resolved first)
5. Funding platform address
`.skipped` rendering (faded checkmark, secondary text) distinguishes
"we passed this step but didn't need it" from "didn't happen yet"
and from "still active". Exactly one of step 3 / 4 is skipped on
every successful resolution; the discriminator is the lock's
terminal `statusRaw`:
- statusRaw == 2 (InstantSendLocked) → step 4 skipped
- statusRaw == 3 (ChainLocked) → step 3 skipped
(either via IS-timeout fallback or the direct
`last_applied_chain_lock` path)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verifier output at 06c1930. Of the 10 prior findings, 1 (in-flight controller restorability after sheet dismissal) is FIXED by the new PendingPlatformFundingsList surface; the other 9 reproduce at unchanged line ranges. New delta is SwiftExampleApp-only — three new findings in the recipient-stamping + bech32m + pending-list code: the bech32m renderer hardcodes the testnet HRP regardless of wallet network, backfillRecipientOnConsumedLock matches consumed rows by status alone (race across concurrent fundings) and hardcodes the address-type byte, and the new PendingPlatformFundingsList anti-join is a no-op (controllers don't carry the outpoint they're driving) so the same lock can appear twice. Carrying forward 7 still-valid prior findings + 3 new ones; budget overflow dropped: inline 300s timeout, unwrap_or(0) address_index, parseOutPoint inverse coupling. No blocking issues.
🟡 8 suggestion(s) | 💬 2 nitpick(s)
10 additional finding(s)
suggestion: Swift preflight accepts FundingRecipient.addressType values that Rust then rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 06c1930 (lines 594-613, unchanged from 9efcf6e): fundFromCoreAssetLockPreflight validates recipients.isEmpty, the single-nil-remainder invariant, and hash.count == 20, but never inspects addressType. Both fundFromCoreAssetLock and resumeFundFromAssetLock forward addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress only accepts 0 and the orchestrator additionally enforces P2PKH-only at validate_recipient_addresses. A caller building a P2SH FundingRecipient therefore passes the synchronous preflight, spins up the Task, marshals the handle, and only then surfaces a generic invalid-parameter error from Rust — defeating the documented purpose of the preflight (deliver a synchronous error before paying for the Task detach). This is also load-bearing for this PR's new backfillRecipientOnConsumedLock, which also hardcodes type=0 — both surfaces would need to be extended in lockstep if P2SH is ever wired up. Either reject addressType != 0 in the preflight, or drop the byte from the public Swift surface until Rust supports it.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Duplicate funding recipients silently collapse in FFI BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 221)
Verified at 06c1930: line 238 calls address_map.insert(addr, balance) and discards the return value, so two FFI rows for the same PlatformAddress silently overwrite each other at the C ABI boundary. Two concrete consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array the Swift caller built — Swift derives the remainder index from that pre-collapse array (ManagedPlatformAddressWallet.swift:460-465, 558-563), so after dedup Rust may see a different recipient set and a different None remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder target can shift, redirecting fees and remainder credits silently. BTreeMap::insert's return value already exposes the collision at zero cost — reject explicitly at the boundary so the input shape the caller passed is the input shape the orchestrator executes.
suggestion: Progress section mis-attributes asset-lock state when two address-fundings run concurrently on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 51)
Verified at 06c1930 (lines 60-66 unchanged): AddressFundingProgressSection.init builds _activeLocks with a #Predicate scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc, and the body drives currentStep, broadcastSubStep, and isInChainLockFallback from activeLocks.first (109-127, 145-149, 154-163). AddressFundingController is keyed (walletId, platformAccountIndex, recipientHash) precisely to allow many concurrent fundings on one wallet, but PersistentAssetLock carries no recipient-hash linkage during the in-flight phase — only outPointHex, walletId, fundingTypeRaw, statusRaw, updatedAt. The new recipientPlatformAddressHash column added in this PR is populated only after Consumed (statusRaw == 4), so it doesn't disambiguate during statusRaw ∈ [0,3]. Concrete consequence: Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast. The new PendingPlatformFundingsList explicitly invites concurrent fundings to coexist on one wallet, so this matters more than before. Fix: (a) propagate the asset-lock outpoint into AddressFundingController so the predicate can scope by outpoint, or (b) populate recipientPlatformAddressHash on the row at broadcast time, not just at consume.
suggestion: Bech32m platform-address renderer hardcodes 'tdash' HRP, mis-renders on mainnet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (line 2012)
New at 06c1930. Verified at lines 2015-2021: networkHRP() unconditionally returns "tdash" with an inline comment that 'Wallet row lookup is best-effort; we keep the explorer self-contained here.' The Rust canonical encoder (packages/rs-dpp/src/address_funds/platform_address.rs) picks the HRP from hrp_for_network(network) — dash on mainnet, tdash on testnet/devnet/regtest. Consequence: on mainnet the storage explorer renders funded platform-address rows as tdash1q…. A user copying that string back through any Rust-owned tool (CLI, SDK) gets rejected by PlatformAddress::from_bech32m_string because the HRP doesn't match the network — the round-trip Swift↔Rust silently breaks at the explorer boundary, which is exactly the surface users open to audit a funding when something looks wrong. record.walletId is already on the row; a FetchDescriptor<PersistentWallet>(predicate: walletId == record.walletId) lookup at render time would surface the network. Better still per the Swift SDK CLAUDE.md rule ('No re-implementing protocol constants … as Swift mirrors'), expose a single FFI helper that calls to_bech32m_string and returns the string for display, since the entire bech32mEncode/convertBits/type-byte mapping is a hand-rolled Swift mirror of canonical Rust logic with no round-trip test pinning them.
suggestion: backfillRecipientOnConsumedLock matches consumed rows by status alone and hardcodes P2PKH, races on concurrent fundings
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 629)
New at 06c1930. Verified at lines 629-665. Two distinct gaps in the same routine: (1) Line 640 hardcodes let recipientType: UInt8 = 0 // P2PKH discriminant., but the originally-FFI-submitted addressType byte (the thing this back-fill exists to document) is never captured on the controller. Today benign because the FFI rejects type != 0, but the moment either side relaxes (Swift preflight or Rust FFI) the storage explorer will display a wrong type discriminant for a successful funding. (2) Lines 641-649 match solely on (walletId, fundingTypeRaw==4, statusRaw==4, recipientPlatformAddressHash==nil) newest-first. With two back-to-back fundings on the same wallet, persister-callback ordering (Rust-side background actor) and SwiftUI .onChange ordering (MainActor continuation) are independent — Funding A's lock can reach Consumed first while Funding B's controller publishes .completed to SwiftUI first, stamping A's lock with B's recipient hash, and vice versa on A's later .completed. The in-source comment ('FIFO by completion time') asserts an invariant the runtime doesn't actually guarantee, and a single misordered match permanently mis-attributes both rows (the back-fill never reconsiders an already-stamped row). The downstream impact is the storage explorer's audit trail showing fundings going to addresses they didn't go to. Fix at the source: have Rust return the consumed outpoint(s) on the FFI changeset (the orchestrator already has tracked_out_point at fund_with_funding.rs:209), then back-stamp by outpoint instead of by status. Stash recipient.addressType on AddressFundingController while you're there.
suggestion: PendingPlatformFundingsList anti-join is a no-op — same lock can render twice and be resumed while in flight
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)
New at 06c1930. Verified at lines 45-58: the construction of the exclusion set for resumableLocks is gated by inFlight.compactMap { _ in nil } — every entry returns nil, so the set is always empty. The inline comment acknowledges this ('The de-dupe set therefore never has entries today'). The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but for an in-flight controller whose asset lock has just been broadcast, the same outpoint sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens FundPlatformAddressView in resume mode against the same outpoint and the user can pick a different recipient than the original in-flight flow chose; the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch it because the recipient hash differs. Two concurrent FFI calls then race against the same asset-lock outpoint. Bounded — Platform's asset-lock-consumption check rejects the second top_up ST so no double-credit — but the user pays full FFI roundtrip cost on a guaranteed-to-fail submission, and the failure surfaces as a generic Platform reject which masks the duplicate-attempt cause. Fix per the comment's own roadmap: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.
suggestion: Funding success path mutates in-memory balances but never pushes the changeset through the persister
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 205)
Verified at 06c1930 (lines 205-223 unchanged): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount and returns a PlatformAddressChangeSet, but the orchestrator returns immediately after consume_asset_lock without invoking self.persister.store(cs.clone().into()). The transfer path explicitly persists with an inline comment that stale persisted balances would otherwise break initialize_from_persisted and auto_select_inputs on next process start. The Rust crate owns the persister, so the Swift caller cannot compensate from the returned changeset. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local wallet doesn't know — affecting auto-input selection and any UI reading balances from the persisted SwiftData store. The new resumable-orphan surface in this PR doesn't help here: by the time we reach this code path the lock is already consumed, so there's nothing to resume — only the new credit balance is lost from persistence.
suggestion: No direct tests for fund_addresses_with_funding orchestration branches
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
Verified at 06c1930: submit_with_cl_height_retry has an explicit regression-pinning test and DPP/SDK primitives are exercised elsewhere, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. The most fragile branch is the IS-rejection fallback: it intentionally restarts submit_with_cl_height_retry from the original settings value — because PutSettings is Copy, the bumped value from the first call is discarded, which is required because the proof type changes, the ST hash differs, and Tenderdash's invalid-tx cache no longer applies. A future refactor switching to &mut settings would silently break that retry-cache-bypass invariant. A targeted #[tokio::test] (with start_paused = true) covering (a) IS-timeout → CL upgrade, (b) IS-rejection → CL upgrade, (c) consume-on-success, and (d) resume-from-existing-asset-lock would pin the orchestration shape. PR description marks end-to-end testnet validation as still pending.
nitpick: Doc claim of 'shared with the legacy raw-private-key FFI' is inaccurate
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)
Verified at 06c1930 (lines 214-216 unchanged): the doc on decode_funding_addresses claims it is 'Shared with the legacy raw-private-key [crate::platform_addresses::fund_from_asset_lock] FFI so both entry points agree on the wire shape.' Confirmed false — platform_addresses/fund_from_asset_lock.rs:35-44 still has its own inline for entry in from_raw_parts(...) decode loop and never calls decode_funding_addresses. The two FFI decode paths therefore do not share logic, so any future tightening of one (duplicate rejection, null handling, P2SH boundary check, error wording) will silently diverge across the boundary. Either route the legacy FFI through decode_funding_addresses to make the comment true, or correct the comment.
nitpick: decode_funding_addresses relies on caller's check_ptr! for null-safety at an FFI trust boundary
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)
Verified at 06c1930 (lines 218-226): the // Safety doc requires non-null addresses, but the body unconditionally calls std::slice::from_raw_parts(addresses, addresses_count). Per the slice contract this is UB on a null data pointer even when count == 0. Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. But the helper is pub(super) and sits on a C ABI trust boundary consuming caller-supplied raw pointers; a future intra-module caller that omits check_ptr! — or fast-paths addresses_count == 0 on the hunch that 'empty can't matter' — would silently introduce UB the optimizer is free to compile into a memory-safety bug. Sibling helpers in this crate (parse_outputs, parse_explicit_inputs) defensively null-check internally; aligning here costs nothing and removes a sharp edge from a security boundary.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…ngs A) Three HIGH findings from the parallel code review, batched as quick safety + UX wins: H4 — Bech32m HRP hardcoded to "tdash". The recipient-address display on `AssetLockStorageDetailView` rendered every address with the testnet HRP, producing a string that's bech32m-valid but decodes to the wrong `Network` on mainnet. Adds a `@Query` for the asset lock's owning `PersistentWallet` and reads `wallet.network` to pick `dash` vs `tdash` per DIP-0018. The earlier fallback string is retained as the orphan-wallet-row case (legacy rows without the relationship populated) — that case is already non-functional so the fallback HRP is inconsequential. H5 — Missing `// SAFETY:` comments on the resume FFI's `unsafe` blocks (`platform_address_wallet_resume_fund_with_existing_asset_lock_signer`). Sibling at line 90 already had the comment; resume sibling didn't. Trivial 1-line parity fix. H3 — `.swipeActions` on `PendingPlatformFundingRow` was dead. The modifier only fires when the row is inside a List/Form, but the row renders inside the VStack "Pending Platform Funding" card on the wallet detail screen. A `.failed` controller had no in-app dismissal path — relaunch was the only way to clear it. Replaced with an inline trash-icon `Button` next to the row, plus an explicit chevron-right (List would normally auto-render this). Standalone `AddressFundingProgressView`'s `.failed` terminal section was missing its Dismiss button entirely — added one that mirrors the inline `FundPlatformAddressView` variant so both surfaces have a working dismissal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (1)
2044-2127: 💤 Low valueConsider extracting bech32m helpers to a shared utility.
These ~80 lines of bech32m encoding logic are correct but could be reused elsewhere (e.g., if other views need to display platform addresses). For now this is acceptable as display-only code in an example app, but if platform address rendering spreads to more surfaces, extracting to a shared
Bech32mutility would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift` around lines 2044 - 2127, This code block duplicates bech32m logic and should be extracted to a reusable utility: create a new Bech32m (or Bech32mUtils) module/class and move the functions convertBits, bech32mEncode, bech32mCreateChecksum, bech32mHRPExpand and bech32mPolymod into it as public/static helpers; update StorageRecordDetailViews to import/use that utility (call Bech32m.bech32mEncode(...) etc.), keep signatures and behavior identical, and add a small unit test or usage example to ensure parity after refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- Around line 2044-2127: This code block duplicates bech32m logic and should be
extracted to a reusable utility: create a new Bech32m (or Bech32mUtils)
module/class and move the functions convertBits, bech32mEncode,
bech32mCreateChecksum, bech32mHRPExpand and bech32mPolymod into it as
public/static helpers; update StorageRecordDetailViews to import/use that
utility (call Bech32m.bech32mEncode(...) etc.), keep signatures and behavior
identical, and add a small unit test or usage example to ensure parity after
refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c339ff33-ba31-4939-9f70-429cf78450e0
📒 Files selected for processing (7)
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift
- packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs
…findings B)
H2 — Back-fill was hardcoding the recipient address type to P2PKH
(`recipientType: UInt8 = 0`) instead of carrying the user's actual
`recipient.addressType`. Today every platform address the wallet
emits is P2PKH so the latent bug isn't visible, but the value
persists on `PersistentAssetLock.recipientPlatformAddressType` and
feeds the bech32m encoder's type byte (`0xb0` vs `0x80`). Any
future P2SH funding would have been silently mis-tagged forever.
Plumbed `recipientType` through `AddressFundingController` and
`AddressFundingCoordinator.startFunding(...)` so the back-fill
stamps the real value.
H10 — Back-fill matched on `(walletId, fundingType==4, status==4,
recipientHash==nil)` newest-first. Two concurrent fundings on the
same wallet that Consume in close succession could trip the
`updatedAt` ordering and stamp the wrong row — the in-code comment
admitted the FIFO claim was hand-wavy. Replaced with a deterministic
snapshot-delta:
- `submit()` captures `preSubmitConsumedOutpoints` (every
Consumed address-funding outpoint on the wallet RIGHT NOW).
- `.onChange(.completed)` recomputes the set and filters the
unrecipiented Consumed rows down to those NOT in the snapshot
— those are the genuinely new rows.
- Exactly one new outpoint → stamp it.
- Zero new outpoints → persister lag; skip, next funding's delta
will pick this up.
- Multiple new outpoints → ambiguous race; refuse to stamp
either, better unknown than wrong.
- Snapshot missing → fall back to the legacy newest-heuristic
(kept for any future caller that wires the coordinator
directly without going through the view).
Also addressed the swift-ios reviewer's MEDIUM finding about
duplicate retention-sweep `Task`s: the `.idle`/`.failed` retry
branch in `startFunding` was calling `scheduleRetentionSweep`
again, spawning a second 30s poll loop against the same
controller. Sweep is now scheduled once per controller (on
creation only).
Also replaced the previous `print(error)` swallow + `try?` save
swallow with explicit `⚠️ `-prefixed prints matching the surrounding
app's logging idiom. Still not OSLog-grade but at least
grep-discoverable in the console output (was invisible on the
silent-failure-hunter pass).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…findings C)
H1 (silent-failure hunter) — `write_address_balances_changeset` had
two fallback paths that silently masked a Platform proof contract
violation as a successful zero-credit funding:
1. A recipient with `maybe_info == None` in `AddressInfos` was
mapped to `AddressFunds { balance: 0, nonce: 0 }` and pushed
onto the changeset. The asset lock would be Consumed against
a recipient whose ledger row shows "credited 0".
2. A recipient whose hash wasn't found in the account's address
pool fell back to `address_index = 0` via `.unwrap_or(0)` —
silently mis-attributing the credits to whatever real address
happened to live at slot 0.
Replaced both with `tracing::error!` + skip. The asset lock has
already Consumed at this point, so propagating an `Err` would
mis-report the protocol outcome — but writing the bad row is
worse than writing nothing. Operators see the drift in logs;
storage explorer shows "—" for the recipient column instead of a
silently wrong value.
H9 (reliability) — Added a post-condition assertion at the top of
Step 4 that errors out when `address_infos.is_empty()` for a
non-empty recipient set. An empty Ok response would have written
no changeset rows AND consumed the asset lock, producing a
terminal state where the user paid for an asset lock and got
nothing in return. This refuses the consume entirely so the lock
stays available for a retry.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I revalidated the cumulative review at 0753138fffcaffaf02ada11275ff1d9bba5d8b59 against the checked-out source. None of the 10 prior findings were fixed; all remain reproducible at this head, and the AddressFundingProgressView.swift rewrite introduces one additional UI-history regression. To stay within the 10-comment budget, I merged the stale decode_funding_addresses doc-comment nit into the concrete FFI decoder drift finding below.
🟡 9 suggestion(s) | 💬 1 nitpick(s)
10 additional finding(s)
suggestion: Swift preflight still accepts recipient address types that Rust rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
fundFromCoreAssetLockPreflight checks only non-empty input, the single-nil remainder invariant, and hash.count == 20. Both funding entry points then pass FundingRecipient.addressType through unchanged, but PlatformAddress::try_from in Rust accepts only address_type == 0, and validate_recipient_addresses rejects non-P2PKH recipients as well. That means unsupported recipient types pass the synchronous Swift preflight and fail only after the async FFI setup and Rust call begin, which defeats the point of the preflight guard.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: FFI funding-address decode still mutates the caller's recipient set and has already drifted from the legacy entry point
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 210)
decode_funding_addresses inserts each entry into a BTreeMap and ignores the return value from insert, so duplicate platform addresses are silently overwritten at the ABI boundary. That changes the recipient set after Swift has already chosen the fee strategy and the single None remainder target. The helper's own comment claims this logic is shared with the legacy raw-key FFI path, but platform_addresses/fund_from_asset_lock.rs still has a separate inline decode loop, so the two entry points can keep drifting on duplicate rejection and other validation rules.
suggestion: Progress view still binds to the wrong asset-lock when two address fundings run concurrently on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 61)
The @Query is still scoped only by walletId and fundingTypeRaw == 4, and every step computation reads activeLocks.first. AddressFundingController is keyed by (walletId, platformAccountIndex, recipientHash), but PersistentAssetLock still does not carry that discriminator while the funding is in flight. If two address top-ups run concurrently on the same wallet, both progress views observe the same lock set and whichever row updated most recently drives both steppers.
suggestion: Platform-address renderer still hardcodes the testnet HRP
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift (line 2012)
networkHRP() always returns "tdash". The storage view therefore renders mainnet recipient platform addresses with a testnet bech32m prefix, which is not just cosmetic: copying that string back into Rust-side parsing fails on network mismatch. The surrounding comment says the code should derive the HRP from the owning wallet, but this implementation never does.
suggestion: Consumed-lock backfill still races concurrent completions and overwrites the recipient type
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 621)
backfillRecipientOnConsumedLock still selects the newest row matching (walletId, fundingTypeRaw == 4, statusRaw == 4, recipientPlatformAddressHash == nil) and stamps that row with the current controller's recipient hash. If two fundings complete close together on the same wallet, callback ordering can stamp the wrong consumed lock. The method also hardcodes recipientPlatformAddressType = 0, so even if another address type is submitted later the stored audit trail is forced to P2PKH.
suggestion: Pending fundings anti-join is still disabled, so the same lock can be shown twice
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)
The exclusion set is built with inFlight.compactMap { _ in nil }, so it is always empty. That means a lock already owned by an in-flight controller can also appear in the resumable-orphan list while its persisted row is still in statuses 1...3. The user can then trigger a second resume submission against the same outpoint and hit a later duplicate rejection instead of being prevented at the UI layer.
suggestion: Successful address funding still updates only in-memory balances and never persists the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 201)
After a successful top-up, fund_addresses_with_funding writes the new balances into the managed account and returns the PlatformAddressChangeSet, but it never calls self.persister.store(cs.clone().into()). Other mutating address flows such as transfer persist immediately for exactly this reason. If the process dies before a later sync runs, the platform accepted the top-up but persisted local state still reflects the old balances, which can break restart-time balance display and input selection.
suggestion: `fund_addresses_with_funding` still has no direct orchestration tests
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
There are still no tests that exercise fund_addresses_with_funding itself. The method owns the recipient preflight, funding resolution, IS-timeout fallback, IS-rejection fallback, CL-height retry wiring, balance writeback, and consume-on-success cleanup. The repository only has a dedicated test for the lower-level submit_with_cl_height_retry helper, so the most failure-prone branches in this new orchestrator remain unpinned.
nitpick: Unsafe FFI decoder still relies on callers to null-check before `from_raw_parts`
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 218)
decode_funding_addresses documents that addresses must be non-null, but it immediately calls std::slice::from_raw_parts(addresses, addresses_count) without enforcing that precondition itself. The current extern entry points do call check_ptr!(addresses) first, so this is not an immediate exploit in the shipped API, but the helper is reusable within the module and a future caller can reintroduce UB by forgetting that outer guard. This null check belongs in the unsafe helper itself, not in every caller's memory.
pub(super) unsafe fn decode_funding_addresses(
addresses: *const FundingAddressEntryFFI,
addresses_count: usize,
) -> Result<BTreeMap<PlatformAddress, Option<dpp::fee::Credits>>, PlatformWalletFFIResult> {
if addresses.is_null() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"addresses pointer is null",
));
}
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance {
Some(entry.balance)
} else {
None
};
address_map.insert(addr, balance);
}
Ok(address_map)
}
suggestion: The rewritten 5-step progress view now rewrites timed-out InstantSend waits as if they were skipped
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 176)
step3WasSkipped now returns lock.statusRaw != 2, and stepState uses that once the flow advances past step 3. On the timeout-fallback path, step 3 is actually active for up to instantLockTimeout while the row remains statusRaw == 1; after the fallback succeeds and the row becomes statusRaw == 3, the UI retroactively fades that completed wait as a skipped step. The new implementation therefore loses history for the common IS-timeout path and misrepresents what the user just waited through.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…findings D) H7 (reliability) — Added a Latency budget + Cancellation section to `fund_addresses_with_funding`'s doc-comment. Worst-case wall time stacks at ~690s on the IS-rejection branch; happy path is single- digit seconds. The function is explicitly documented as non-cancellation-safe; the Swift `AddressFundingController.task` deliberately doesn't call `.cancel()` (FFI dropping mid-call would leave partial state). UI dismissal hides the progress view without aborting the work; resume picks the lock back up via `FromExistingAssetLock`. H6 (reliability) — `consume_asset_lock` failure was logged as `warn` regardless of the underlying error. The expected failure mode is `WalletNotFound` (the wallet handle vanished between submit and cleanup) — that's benign because Platform's deterministic "lock already consumed" rejection handles the resume-attempt user-visible recovery path. Anything else is an unexpected invariant violation and should land at `error` level. Pattern-matched the error and split the log severity accordingly. H8 (reliability) — Documented `submit_with_cl_height_retry`'s retry scope. Only consensus 10506 is retried at THIS layer; every other `dash_sdk::Error` (transport, mempool, DAPI 5xx) falls through immediately because the `rs-dapi-client` layer underneath already implements per-request retry + endpoint rotation for transport failures. Adding a second generic-retry layer here would over-retry and risk double-submission. Rationale + when-to-widen guidance recorded in the doc comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five MEDIUM findings batched into a single commit: F6 (correctness) — `submit()` silently no-op'd when the selected recipient address was no longer in `recipientCandidates` (the row flipped to `isUsed = true` between user-tap and submit body). Now surfaces a typed `submitError` so the user sees "the address is no longer available; pick a fresh one" instead of an unresponsive button. F4 (correctness) — IS-rejection branch left the tracked asset-lock status at `InstantSendLocked` even after building a fresh CL proof. If the second `submit_with_cl_height_retry` then failed, the persisted status was stale (CL proof was attached internally but discarded; the row carried the rejected IS proof). The storage explorer + Resume UI both misreported the recovery state. Now calls `advance_asset_lock_status(ChainLocked, Some(chain_proof))` between the upgrade and the second submit so the row accurately reflects the lock's CL-attached state regardless of which way the submit goes. swift-ios #7 — "No funded Core (BIP44 standard) accounts on this wallet" empty-state copy fired even when zero-balance accounts existed. The picker shows all BIP44 standard accounts (incl. zero balance) so the "funded" qualifier was a lie. Dropped it; the picker's balance column tells the user what's spendable. rust-quality #3 — `drive-proof-verifier` Cargo dep was missing `default-features = false`. Consistency with every other path-dep in this crate; harmless today (default feature set is empty) but insulates from upstream feature additions. blockchain-security M2 — `decode_funding_addresses` zero-count short-circuit. The FFI's `slice::from_raw_parts(ptr, 0)` contract is technically sound even with a dangling non-null sentinel, but the explicit early-return makes the safety doc trivially satisfied + skips a function call. Side change: `AssetLockManager::queue_asset_lock_changeset` visibility widened from `pub(super)` to `pub(crate)` so the orchestrated funding flow can pair an `advance_asset_lock_status` call with a flush without going through the asset-lock module boundary. Still crate-private; no consumer-facing API change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried forward 9 still-valid prior findings against 0a0ba19. The latest delta (SAFETY comments, mainnet HRP fix, dismissal paths) introduced no new defects; it resolved prior finding #4 (StorageRecordDetailViews now derives bech32m HRP from the owning wallet, returning dash on mainnet and tdash otherwise). All other prior findings reproduce at current line ranges and remain unaddressed.
🟡 8 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
suggestion: Swift preflight accepts recipient address types that Rust will reject
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 0a0ba19: fundFromCoreAssetLockPreflight only checks non-empty input, the single-nil remainder invariant, and hash.count == 20 — it does not validate addressType. Both funding entry points then marshal FundingRecipient.addressType unchanged into PlatformAddressFFI, but TryFrom<PlatformAddressFFI> for PlatformAddress (packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-43) only accepts address_type == 0, and the Rust wallet path rejects non-P2PKH again in validate_recipient_addresses (packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs:335-341). Unsupported recipient types pass the synchronous Swift guard and fail only after Task setup + the Rust call begin, which defeats the stated purpose of the preflight. Add a guard r.addressType == 0 check next to the hash.count guard.
suggestion: FFI funding-address decode silently collapses duplicate platform addresses
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 223)
Verified at 0a0ba19 (lines 227-241): decode_funding_addresses inserts each FFI entry into a BTreeMap<PlatformAddress, Option<Credits>> and ignores insert's return value. If the caller passes the same PlatformAddress twice, the later row silently overwrites the earlier one at the ABI boundary. Swift computes its remainder output index and fee strategy from the ordered ffiAddresses array before the FFI call, so this is input-shape drift across the boundary — Rust can execute a different recipient set than Swift sized. The header comment also still claims this decoder is shared with the legacy raw-key FFI path, but fund_from_asset_lock.rs:35-44 keeps a separate inline decode loop, so the two entry points can continue diverging on validation. Surface a structured ErrorInvalidParameter on duplicate insert and route both entry points through this helper to close the drift.
suggestion: Progress view cannot disambiguate concurrent address fundings on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 58)
Verified at 0a0ba19: the @Query is scoped only by walletId and fundingTypeRaw == 4, and the step logic derives state from activeLocks.first (used by currentStep, step3WasSkipped, step4WasSkipped). AddressFundingController is keyed by (walletId, platformAccountIndex, recipientHash), but PersistentAssetLock has no equivalent per-recipient/outpoint discriminator populated while the funding is in flight (the recipient hash is documented as being populated only after success). With two concurrent address top-ups on the same wallet, both progress views observe the same lock set, and whichever row most recently sorted to first drives both steppers. Plumb the asset-lock outpoint into the controller after broadcast and filter the query by it.
suggestion: Consumed-lock backfill races concurrent completions and hardcodes recipient type
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 629)
Verified at 0a0ba19: backfillRecipientOnConsumedLock fetches the newest row matching (walletId, fundingTypeRaw == 4, statusRaw == 4, recipientPlatformAddressHash == nil) (fetchLimit 1, sorted by updatedAt desc) and stamps it with the current controller's recipientHash and a hardcoded recipientPlatformAddressType = 0. When two fundings complete close together, SwiftData persistence ordering is independent of controller completion ordering, so one controller can stamp the other funding's consumed row. The hardcoded P2PKH type byte also locks the storage-explorer audit trail to P2PKH even if the FFI eventually accepts another address type. The comment acknowledges this; the fix is to thread the outpoint through AddressFundingController so the back-fill can address its own row directly, and to capture the original FFI addressType rather than hardcoding 0.
suggestion: Pending fundings anti-join is a no-op so the same lock can be resumed twice
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)
Verified at 0a0ba19: orphans = resumableLocks(excludingControllerOutpoints: Set(inFlight.compactMap { _ in nil })) — the exclusion set is always empty by construction. The inline comment acknowledges that controllers don't currently track the outpoint they're driving. Resumable rows are identified by PersistentAssetLock.outPointHex, so the same persisted lock can simultaneously appear as an in-flight controller row and as a resumable orphan. Tapping Resume can launch a second FFI submission against the same Rust-tracked outpoint with a different recipient hash; Platform rejects the duplicate later, but the UI should not have lost the outpoint identity in the first place. Persist the outpoint on the controller after broadcast and de-dupe here.
suggestion: Successful address funding updates only in-memory balances and never persists the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 201)
Verified at 0a0ba19 (lines 205-223): after a successful submit, fund_addresses_with_funding calls write_address_balances_changeset to update ManagedPlatformAccount in memory and returns the resulting PlatformAddressChangeSet, but it never pushes the changeset through self.persister.store(...) before consuming the asset lock. Other mutating address flows such as transfer (packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:145-159) persist immediately for exactly this reason. The Swift caller only receives an ephemeral decoded [UpdatedBalance]; on restart it rebuilds from persistence, not from that transient array. If the process dies before the next sync, Platform has accepted the top-up but persisted wallet state still reflects the old balances, which can poison initialize_from_persisted and downstream input selection.
suggestion: fund_addresses_with_funding has no direct orchestration tests
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 88)
Verified at 0a0ba19: fund_addresses_with_funding owns the end-to-end orchestration — recipient preflight, funding resolution, InstantSend-timeout fallback, IS-rejection fallback (the second submit_with_cl_height_retry(settings, ...) call intentionally restarts from the original PutSettings after switching proof type), CL-height retry wiring, balance writeback, and consume_asset_lock cleanup. The repository only pins the lower-level submit_with_cl_height_retry helper in wallet/asset_lock/orchestration.rs; there is no direct test exercising this method's own control flow. A future refactor that changes that PutSettings ownership pattern or the IS→CL switchover would be easy to write and hard to catch. Add targeted tests covering the IS-timeout, IS-rejection, and success-with-cleanup branches.
suggestion: 5-step progress view rewrites timed-out InstantSend waits as skipped
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 176)
Verified at 0a0ba19: step3WasSkipped returns lock.statusRaw != 2, and stepState uses it once the flow advances past step 3. On the IS-timeout fallback path, step 3 is actually active for up to instantLockTimeout (300 s) while the row remains statusRaw == 1; after the CL fallback succeeds and the row becomes statusRaw == 3, the UI retroactively fades the completed IS wait as a skipped step. The current doc comment explains the simplification, but the history shown to the user is still inaccurate on the common IS-timeout path. Either distinguish 'IS timed out → fell back to CL' from 'CL came in directly without IS attempt' using broadcastSubStep's elapsed value at completion, or relabel step 3 so the skipped-state isn't misleading.
nitpick: Unsafe FFI decoder does not null-check before from_raw_parts
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 220)
Verified at 0a0ba19 (lines 223-228): decode_funding_addresses is unsafe pub(super) and immediately calls std::slice::from_raw_parts(addresses, addresses_count) without checking addresses.is_null(). Under Rust's slice contract, from_raw_parts(NULL, 0) is UB. The two current exported entry points call check_ptr!(addresses) first, so the shipped paths are sound today, but this helper sits exactly at the trust boundary for the raw address array and is reusable within the module — one forgotten outer guard reintroduces UB with no type-system resistance. Sibling parsers in platform_address_types.rs defend themselves internally, which is the safer shape. Move the null check inside the helper.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Three `tracing::debug!` lines per method (entry, input/output counts, exit) on `try_from_asset_lock_with_signer_and_private_key` + `try_from_asset_lock_with_signers`. Boilerplate carried over from the legacy method that predates this PR — non-load-bearing, identical pattern to function entry/exit which tracing spans cover when anyone instruments the call site. The counts in particular are derivable from any wrapping span's fields. Drop both pairs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 43efe2e. The delta (commits 3fd355b, 0279557, 431c809, 43efe2e) resolves prior #4 (recipient back-fill race + hardcoded P2PKH type — now snapshot-delta + controller.recipientType) and the dominant UB hole of prior #9 (count==0 short-circuit in decode_funding_addresses). Prior #8 (step3WasSkipped rendering for IS-timeout) is dropped: the new doc-comments at AddressFundingProgressView.swift:176-194 and the .skipped enum doc (lines 217-222) make this the explicit design discriminator between IS-resolved and CL-resolved paths. Seven prior findings still reproduce and are carried forward; two new low-to-medium findings from the delta: the snapshot-delta back-fill's multi-match branch permanently strands both rows unstamped because .completed is terminal, and the new post-condition catches all-empty address_infos but not partial proofs (lock is still consumed with one recipient silently dropped). No blockers. No CodeRabbit context to react to.
🟡 8 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
suggestion: Swift preflight still accepts FundingRecipient.addressType values Rust rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 43efe2e (lines 594-613, unchanged in this delta): fundFromCoreAssetLockPreflight validates non-emptiness, the single-nil remainder invariant, and hash.count == 20, but never inspects addressType. Both entry points then marshal FundingRecipient.addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress accepts only address_type == 0, and validate_recipient_addresses enforces P2PKH-only. Unsupported types pass the synchronous preflight, spin up the Task, marshal the handle, and only then surface a generic invalid-parameter error from Rust — defeating the documented purpose of the preflight (deliver a synchronous error before paying for the Task detach). This is also now load-bearing for the new recipientType plumbing: a non-zero addressType that bypasses validation here taints the recipientPlatformAddressType audit field stamped by the back-fill. Mirror the Rust invariant explicitly next to the hash.count guard.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Duplicate funding recipients silently collapse in FFI BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 237)
Verified at 43efe2e: line 250 address_map.insert(addr, balance); discards the Option returned by BTreeMap::insert. The new empty-count short-circuit doesn't change this path. Two consequences at the C ABI trust boundary: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array Swift sized the fee strategy and the single-None remainder against — after dedup Rust may see a different recipient set and a different remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder slot can shift after dedup, redirecting fees and remainder credits silently. The doc comment claims this helper is shared with the legacy raw-key FFI, but fund_from_asset_lock.rs:35-44 still has its own inline decode loop — the two FFI decode paths therefore do not share logic and will silently diverge on any future tightening. Reject duplicates explicitly with ErrorInvalidParameter so the input shape the caller passes is the input shape the orchestrator executes.
suggestion: Progress view still cannot disambiguate concurrent address fundings on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 58)
Verified at 43efe2e (lines 67-72, unchanged): _activeLocks is Query-scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc; currentStep, step3WasSkipped, step4WasSkipped, broadcastSubStep all key off activeLocks.first. AddressFundingController is keyed (walletId, platformAccountIndex, recipientHash) precisely to allow concurrent fundings, but PersistentAssetLock carries no recipient-hash discriminator during the in-flight phase (recipientPlatformAddressHash is stamped only post-Consumed). Two concurrent address top-ups on one wallet observe the same lock set, and whichever row most recently sorted to first drives both steppers — Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast. The new PendingPlatformFundingsList explicitly invites concurrent fundings to coexist, so this matters more, not less. Fix shape: propagate the asset-lock outpoint into the controller after broadcast and scope the query by it (same plumbing as finding #4).
suggestion: PendingPlatformFundingsList anti-join is still a no-op — same lock can render twice and be resumed while in flight
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)
Verified at 43efe2e (lines 45-58, unchanged): orphans = resumableLocks(excludingControllerOutpoints: Set(inFlight.compactMap { _ in nil })) — every entry returns nil, so the exclusion set is always empty. The inline comment acknowledges this. The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but an in-flight controller whose asset lock has just broadcast sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens FundPlatformAddressView in resume mode against the same outpoint; the user can pick a different recipient than the original flow, and the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch the duplicate. Bounded (Platform deterministically rejects the duplicate ST) but the user pays a full FFI round-trip on a guaranteed-to-fail submission. Same fix path as finding #3: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.
suggestion: Successful address funding still updates only in-memory balances and never calls persister.store on the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 285)
Verified at 43efe2e (lines 285-328): after a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount in memory and returns PlatformAddressChangeSet, but the orchestrator goes straight to consume_asset_lock and Ok(cs) without invoking self.persister.store(cs.clone().into()). Sister flows persist explicitly: transfer.rs:156 and sync.rs:80 both call self.persister.store(cs.into()). The new IS→CL queue_asset_lock_changeset call DOES persist the asset-lock side of the world, but the address-balance changeset returned to the FFI still never lands in the persister. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local SwiftData store doesn't know the new credit balance — affecting auto-input selection and any UI reading balances from the persisted store. The new consume_asset_lock warn-vs-error classification implicitly assumes the changeset has already been durably recorded — it hasn't.
suggestion: fund_addresses_with_funding still has no direct orchestration tests; new branches landed unpinned
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 127)
Verified at 43efe2e: submit_with_cl_height_retry has an explicit regression-pinning test, but the new orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. This delta added behaviorally distinct branches that are also unpinned: (a) the IS-rejection path now advances tracked status InstantSendLocked → ChainLocked via advance_asset_lock_status + queue_asset_lock_changeset BEFORE the second submit_with_cl_height_retry, load-bearing for the truthful-status-on-double-failure invariant; (b) the empty-address_infos post-condition returning PlatformWalletError::AddressSync(...) instead of consuming the asset lock; (c) the split warn/error logging on consume_asset_lock failure; (d) the per-recipient skip-and-continue path in write_address_balances_changeset. A targeted #[tokio::test(start_paused = true)] set would pin the orchestration shape. PR description still marks end-to-end testnet validation as pending.
suggestion: Post-condition catches empty address_infos but not partial — missing recipients are silently skipped and the lock is still consumed
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 267)
New in this delta. The post-condition at lines 275-283 hardens the all-empty case: address_infos.is_empty() returns AddressSync and the asset lock stays Resumable. But the partial case is not symmetric. expected_recipient_count = addresses.len() is captured but never used as an invariant. Downstream in write_address_balances_changeset (lines 360-412), each requested recipient missing from address_infos emits tracing::error! and continues — correctly avoiding the prior 'credited 0' shape — but the orchestrator then proceeds to consume_asset_lock(out_point) unconditionally on the success path. A proof that returns AddressInfo for N-1 of N requested recipients therefore consumes the asset lock with one recipient invisibly dropped from the persisted changeset. User-visible outcome: ST succeeded, asset lock is gone, one recipient never gets a balance row, and the only signal is a tracing::error! (no metric / structured error). Either extend the post-condition to address_infos.len() == addresses.len() and refuse to consume on shortfall, or surface a structured PlatformWalletError::ProofIncomplete variant the caller can react to.
suggestion: Snapshot-delta back-fill 'multi-match → refuse to stamp' leaves both rows permanently unrecipiented
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 691)
New in this delta (3fd355b). Verified at lines 708-731 (default branch of the switch newRows.count): when two address-funding flows resolve Consumed in the same .onChange window, both controllers see newRows.count > 1 and both refuse to stamp. The accompanying doc comment claims controllers 'will retry on their next phase tick … in practice one will land first and the other re-runs against the now-smaller delta.' But .completed is the terminal phase of AddressFundingController.phase; the controller does not keep ticking after it lands there, and .onChange(of: controller?.phase) fires once per transition into .completed. If both controllers' .completed transitions land on the same MainActor turn (or close enough that both .onChange closures run before either back-fill saves), both back-fills see the same multi-element delta, both refuse, and neither row is stamped — permanently. The fallback is UX-only (storage explorer shows '—' on both rows), not a correctness defect. Fix: in the multi-match branch, stamp the row matching the current controller's outpoint if the controller has one — the natural place to surface it is the same plumbing findings #3/#4 want.
nitpick: decode_funding_addresses still relies on callers to null-check when addresses_count > 0
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 220)
Verified at 43efe2e: the new short-circuit at lines 234-236 returns early when addresses_count == 0 and the SAFETY doc was tightened to say a 0-count dangling sentinel is sound. That closes the dominant UB hole prior #9 called out (from_raw_parts(NULL, 0)). When addresses_count > 0 the helper still calls std::slice::from_raw_parts(addresses, addresses_count) without addresses.is_null(). Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. The helper is pub(super) and sits on a C ABI trust boundary; a future intra-module caller that forgets check_ptr! for the non-zero case would reintroduce UB the optimizer is free to compile into a memory-safety bug. Sibling FFI decoders in platform_address_types.rs defensively null-check internally; aligning here costs nothing and removes a sharp edge from a security boundary.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at e27dcba. The delta from 43efe2e is a single commit that only removes tracing::debug! entry/exit logs from AddressFundingFromAssetLockTransition signers — no behavior change, no new defects. All 9 prior findings reconciled against current source: all 9 STILL VALID at unchanged line ranges, none FIXED, none OUTDATED. Carrying every still-valid prior finding forward; no new findings introduced by this delta. No blocking issues; all findings are suggestions or nitpicks.
🟡 8 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
suggestion: Swift preflight accepts FundingRecipient.addressType values Rust rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at e27dcba (lines 594-613, unchanged). fundFromCoreAssetLockPreflight validates non-emptiness, the single-nil remainder invariant, and hash.count == 20, but never inspects addressType. Both fundFromCoreAssetLock and resumeFundFromAssetLock then marshal FundingRecipient.addressType unchanged into PlatformAddressFFI. On the Rust side, TryFrom<PlatformAddressFFI> for PlatformAddress (platform_address_types.rs) accepts only address_type == 0, and validate_recipient_addresses re-enforces P2PKH-only. Unsupported types pass the synchronous preflight, spin up the Task, marshal the handle, and only then surface a generic invalid-parameter error from Rust — defeating the documented purpose of the preflight (synchronous fail-fast before paying for Task detach + handle marshaling). Also load-bearing for the new recipientPlatformAddressType audit field: a non-zero addressType that bypasses this guard taints what the back-fill stamps. Mirror the Rust invariant explicitly next to the hash.count guard.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"FundingRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Duplicate funding recipients silently collapse in FFI BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 237)
Verified at e27dcba (line 250 unchanged). address_map.insert(addr, balance); discards the Option returned by BTreeMap::insert, so duplicate PlatformAddress entries silently overwrite at the C ABI trust boundary. Two consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [FundingRecipient] array Swift sized the fee strategy and the single-None remainder against — after dedup Rust may see a smaller recipient set and a different remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder slot can shift, redirecting fee remainder credits to a different address than the caller computed against, with no typed error crossing the boundary. The doc comment (lines 216-218) claims this helper is shared with the legacy raw-key FFI, but fund_from_asset_lock.rs:35-44 still has its own inline decode loop — the two FFI decoders have already drifted on duplicate handling. Reject duplicates with ErrorInvalidParameter so the input shape the caller passes is the input shape the orchestrator executes, and route both entry points through this helper.
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance {
Some(entry.balance)
} else {
None
};
if address_map.insert(addr, balance).is_some() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"duplicate platform address in funding recipient list".to_string(),
));
}
}
Ok(address_map)
suggestion: Successful address funding never calls persister.store on the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 285)
Verified at e27dcba (lines 285-328 unchanged). After a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount in memory and returns PlatformAddressChangeSet, but the orchestrator goes straight to consume_asset_lock and Ok(cs) without invoking self.persister.store(cs.clone().into()). Sister flows persist explicitly: transfer.rs:156 and sync.rs:80 both call self.persister.store(cs.into()). The IS→CL queue_asset_lock_changeset DOES persist the asset-lock side, but the address-balance changeset returned to the FFI never lands in the persister. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local SwiftData store doesn't know the new credit balance — affecting auto-input selection and any UI reading balances from the persisted store. The consume_asset_lock warn-vs-error classification implicitly assumes the changeset has already been durably recorded; it hasn't.
suggestion: Post-condition catches empty address_infos but not partial — missing recipients silently dropped while the asset lock is consumed
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 267)
Verified at e27dcba (lines 267-326 unchanged). The post-condition (lines 275-283) hardens the all-empty case: address_infos.is_empty() returns AddressSync and the asset lock stays Resumable. The partial case is not symmetric. expected_recipient_count = addresses.len() is captured (line 275) but never compared as an invariant. Downstream in write_address_balances_changeset, each requested recipient missing from address_infos emits tracing::error! and continues, but the orchestrator then proceeds to consume_asset_lock(out_point) unconditionally on the success path. A proof returning AddressInfo for N-1 of N requested recipients therefore consumes the asset lock with one recipient invisibly dropped from the persisted changeset. Threat model: a buggy or compromised DAPI intermediary can stitch a proof that attests for fewer recipients than requested — the client refuses to recurse but still consumes its own asset lock, so the user pays on-chain locked Dash while one recipient's audit row is marked Consumed with no balance changeset. The only signal is a tracing::error! (no metric, no structured error). Either extend the post-condition to address_infos.len() == expected_recipient_count and refuse to consume on shortfall, or surface a structured PlatformWalletError::ProofIncomplete variant the caller can react to.
suggestion: Progress view cannot disambiguate concurrent address fundings on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressFundingProgressView.swift (line 58)
Verified at e27dcba (unchanged). _activeLocks is @Query-scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc; currentStep, step3WasSkipped, step4WasSkipped, and broadcastSubStep all key off activeLocks.first. AddressFundingController is keyed by (walletId, platformAccountIndex, recipientHash) precisely to allow concurrent fundings, but PersistentAssetLock carries no recipient-hash discriminator during the in-flight phase (recipientPlatformAddressHash is stamped only post-Consumed). With two concurrent address top-ups on one wallet, both progress views observe the same lock set and whichever row most recently sorted to first drives both steppers — Funding A's UI can advance to Step 3 because Funding B's lock just transitioned to Broadcast. The PendingPlatformFundingsList explicitly invites concurrent fundings to coexist, so this matters more, not less. Fix shape: plumb the asset-lock outpoint into the controller after broadcast and scope the query by it.
suggestion: PendingPlatformFundingsList anti-join is a no-op — same lock can render twice and be resumed while in flight
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformFundingsList.swift (line 45)
Verified at e27dcba (unchanged). orphans = resumableLocks(excludingControllerOutpoints: Set(inFlight.compactMap { _ in nil })) — every entry returns nil, so the exclusion set is always empty by construction (the inline comment acknowledges this). The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but an in-flight controller whose asset lock has just broadcast sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens FundPlatformAddressView in resume mode against the same outpoint; the user can pick a different recipient, and the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch the duplicate because the second submission keys differently. Bounded (Platform deterministically rejects the duplicate ST) but the user pays a full FFI round-trip on a guaranteed-to-fail submission. Same fix path as the progress-view finding: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.
suggestion: fund_addresses_with_funding has no direct orchestration tests; new branches landed unpinned
packages/rs-platform-wallet/src/wallet/platform_addresses/fund_with_funding.rs (line 127)
Verified at e27dcba. submit_with_cl_height_retry has an explicit regression-pinning test, but the orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. Behaviorally distinct branches added in PR #3671 that remain unpinned: (a) the IS-rejection path advancing tracked status InstantSendLocked → ChainLocked via advance_asset_lock_status + queue_asset_lock_changeset BEFORE the second submit_with_cl_height_retry — load-bearing for the truthful-status-on-double-failure invariant; (b) the empty-address_infos post-condition returning PlatformWalletError::AddressSync(...) instead of consuming the asset lock; (c) the split warn/error logging on consume_asset_lock failure; (d) the per-recipient skip-and-continue path in write_address_balances_changeset. A targeted #[tokio::test(start_paused = true)] set would pin the orchestration shape. PR description still marks end-to-end testnet validation as pending.
suggestion: Snapshot-delta back-fill 'multi-match → refuse to stamp' leaves both rows permanently unrecipiented
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/FundPlatformAddressView.swift (line 708)
Verified at e27dcba (unchanged). In the default branch of the switch newRows.count block, when two address-funding flows resolve Consumed in the same .onChange window, both controllers see newRows.count > 1 and both refuse to stamp. The doc comment claims controllers 'will retry on their next phase tick … in practice one will land first and the other re-runs against the now-smaller delta.' But .completed is the terminal phase of AddressFundingController.phase; the controller does not keep ticking after it lands there, and .onChange(of: controller?.phase) fires once per transition into .completed. If both controllers' .completed transitions land on the same MainActor turn (or close enough that both .onChange closures run before either back-fill saves), both back-fills see the same multi-element delta, both refuse, and neither row is stamped — permanently. The fallback is UX-only (storage explorer shows '—' on both rows), not a correctness defect. Fix: in the multi-match branch, stamp the row matching the current controller's outpoint if the controller has one — same plumbing the progress-view and orphan-list findings want.
nitpick: decode_funding_addresses still relies on outer callers to null-check addresses when addresses_count > 0
packages/rs-platform-wallet-ffi/src/platform_addresses/fund_with_funding.rs (line 226)
Verified at e27dcba (unchanged). The short-circuit at lines 234-236 returns early when addresses_count == 0 and the SAFETY doc was tightened to scope the non-null obligation to that case, closing the dominant UB hole (from_raw_parts(NULL, 0)). When addresses_count > 0 (line 238) the helper still calls std::slice::from_raw_parts(addresses, addresses_count) without verifying !addresses.is_null(). Today's two FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. The helper is pub(super) and sits on a C ABI trust boundary; a future intra-module caller that omits check_ptr! for the non-zero case would reintroduce UB the optimizer is free to compile into a memory-safety bug on attacker-controlled FFI input. Sibling decoders in platform_address_types.rs defensively null-check internally — aligning here removes a sharp edge from a security boundary at zero runtime cost.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
… AssetLockFunding The `identity/types/funding.rs` file became a one-line `pub use ... as IdentityFunding` alias after the refactor that moved the type's body into `wallet/asset_lock/orchestration.rs`. The alias kept existing callers compiling without rename, but every in-tree caller is now updated, so the alias is dead weight. - Deleted `packages/rs-platform-wallet/src/wallet/identity/types/funding.rs`. - Dropped `pub mod funding;` + `pub use funding::IdentityFunding;` from `types/mod.rs`. - Migrated three in-tree call sites (`registration.rs`, `top_up.rs`, and `identity_registration_funded_with_signer.rs` in the FFI) to import `AssetLockFunding` directly from `platform_wallet::wallet::asset_lock`. - Updated public re-exports in `lib.rs` and `wallet/identity/mod.rs`. - Surfaced `AssetLockFunding` directly from the crate root (`platform_wallet::AssetLockFunding`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A handful of comments in this PR's diff referenced what code USED to look like before this PR landed. They were noise — the current shape is what matters; the historical shape lives in git log. - `wallet/asset_lock/orchestration.rs` — dropped the "Historical note" section on `AssetLockFunding` that explained the rename from `IdentityFunding`. The alias was removed in the previous commit too, so the only readers are someone looking at this file fresh; the old name isn't load-bearing for them. - `wallet/platform_addresses/fund_with_funding.rs` — collapsed the two "earlier shape silently mapped..." paragraphs that justified the now-default skip behaviour. Reframed around the WHY (Platform contract violation, recipient must exist in pool) instead of the history (what the previous shape did). - `FundPlatformAddressView.swift` — trimmed the "earlier shape returned nil and the button looked unresponsive" justification on `submit()`'s recipient-staleness guard. Comment now reads as a forward statement about the race window. - `FundPlatformAddressView.swift` — softened the `backfillRecipientOnConsumedLock` "Legacy fallback" branch comment + doc to "snapshot not captured" without the legacy framing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ffix Two renames that hung in the air after the wallet-orchestrated flow became the only path the example app uses: 1. **Remove the legacy raw-private-key wallet path.** `PlatformAddressWallet::fund_from_asset_lock` (and its FFI `platform_address_wallet_fund_from_asset_lock`) only existed as a raw-key wrapper that took a pre-built proof + a 32-byte private key. Every caller in this PR's stack goes through the orchestrated signer-pair path now, so the raw-key surface is dead weight at the platform-wallet layer. Deleted both files. The SDK-level `TopUpAddress::top_up` raw-key trait method stays — it's used by `rs-sdk-ffi/address/transitions/top_up_from_asset_lock.rs`, a different public FFI path that doesn't go through `platform-wallet`. 2. **Drop the `_with_funding` suffix.** The identity side keeps `top_up_identity_with_funding` so it pairs with `register_identity_with_funding`. The address side has no register counterpart (addresses are HD-derived, not registered), so the redundant suffix on `fund_addresses_with_funding` was reading as "fund with funding". Renamed everywhere: Layer | Was | Now ------|-----|---- wallet method | `fund_addresses_with_funding` | `top_up` wallet file | `fund_with_funding.rs` | `top_up.rs` wallet FFI | `platform_address_wallet_fund_with_funding_signer` | `platform_address_wallet_top_up_signer` wallet FFI resume | `..._resume_fund_with_existing_asset_lock_signer` | `..._resume_top_up_with_existing_asset_lock_signer` Swift SDK | `fundFromCoreAssetLock` / `resumeFundFromAssetLock` | `topUpFromCore` / `resumeTopUpFromAssetLock` Swift type | `FundingRecipient` | `TopUpRecipient` App view | `FundPlatformAddressView` | `TopUpPlatformAddressView` App controller | `AddressFundingController` | `AddressTopUpController` App coordinator | `AddressFundingCoordinator` | `AddressTopUpCoordinator` App progress | `AddressFundingProgressView/Section` | `AddressTopUpProgressView/Section` App pending list | `PendingPlatformFundingsList` | `PendingPlatformTopUpsList` Manager extension | `addressFundingCoordinator` | `addressTopUpCoordinator` User strings | "Fund Address" / "Funding…" / "Pending Platform Funding" | "Top Up" / "Topping up…" / "Pending Platform Top Ups" Also collapsed the `write_address_balances_changeset` helper from `pub(super)` to `async fn` (private). It only had one caller after the fund_from_asset_lock removal so the cross-module visibility isn't needed. Workspace cargo check green; 124/124 wallet lib tests pass; iOS sim build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion
Pin the rule explicitly at the top of the model: each funding type
owns its own typed destination-field family; new funding types
follow suit rather than collapsing into a polymorphic
`destinationBytes: Data?` blob.
- Identity (fundingTypeRaw ∈ {0,1,2,3}) → `identityIndexRaw`
- Platform address (fundingTypeRaw == 4) → `recipientPlatformAddress*`
- Shielded (fundingTypeRaw == 5, future) → reserve a
`recipientShielded*` family rather than reusing the platform
address fields or going polymorphic.
This keeps SwiftData predicates typed (`#Predicate { $0.identity\
Index == X }` works) and matches the SwiftData lightweight-
migration story for adding a new funding type later.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three comment blocks in this PR's diff described what was already visible from the surrounding code: - `resumingAssetLock` doc: collapsed from 3 lines describing the type's behaviour (binding → sheet presentation) to one line naming the producer. The `@State PersistentAssetLock?` + the `.sheet(item: $resumingAssetLock)` ten lines down already say the rest. - `walletAssetLocks` doc: dropped entirely. The name + `@Query` + the filter set in init are self-explanatory; the prior comment restated them. - `PendingPlatformTopUpsList` inline comment: dropped. The "collapses to nothing on empty" behaviour is internal to that view (and obvious from its `if !inFlight.isEmpty || ...` outer guard) — the WalletDetailView reader doesn't need it. - Resume `.sheet(item:)` inline comment: dropped. The `resumeFromLock: lock` parameter name is the explanation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CodeRabbit findings from PR review:
**CR-1** (`platform-wallet-ffi/.../top_up.rs:248`) — `decode_funding_addresses`
used `BTreeMap::insert` which silently overwrote duplicates, so a caller
sending two entries for the same recipient address would have one entry's
amount lost. Swift's `topUpFromCorePreflight` already dedupes client-side,
but the FFI is the trust boundary — a future Swift bug or a non-Swift
caller could trip the silent collapse. Now returns
`ErrorInvalidParameter("duplicate platform address in funding request")`
on the second insert.
**CR-5** (`TopUpPlatformAddressView.swift:417`) — `coreAccountOptions`
included zero-balance accounts, and `canSubmit` never compared the
selected account's `balanceDuffs` against `parsedDuffs`. The form
presented a valid-looking submit path for requests guaranteed to fail at
Rust-side UTXO selection. Two fixes:
1. Filter `coreAccountOptions` to `confirmed > 0` so the picker only
shows spendable accounts.
2. Gate `canSubmit` on `selectedCoreAccountBalanceDuffs >= amount`
via a new computed helper.
Empty-state copy also tightened: "No Core (BIP44 standard) accounts on
this wallet." → "No spendable Core (BIP44 standard) accounts on this
wallet." matching the new filter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review at 08c4443 (delta from 43efe2e is pure rename + cleanup: fund_with_funding/fund_addresses_with_funding → top_up, fund_from_asset_lock removed, IdentityFunding alias dropped, Swift AddressFunding→AddressTopUp, FundPlatformAddressView→TopUpPlatformAddressView, debug-log trims, PersistentAssetLock destination doc). No behavior changes. All 9 prior findings reproduce against the renamed source and are carried forward; the 'two FFI decoders drift' sub-claim of finding #2 is resolved by the legacy-FFI deletion, but the surviving decoder still silently collapses duplicates. No new defects introduced by this delta. No blockers.
🟡 8 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
suggestion: Swift preflight accepts TopUpRecipient.addressType values Rust rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 08c4443 (file/lines confirmed; preflight body at 594-613). topUpFromCorePreflight validates non-emptiness, the single-nil remainder invariant, and hash.count == 20, but never inspects addressType. Both entry points marshal TopUpRecipient.addressType unchanged into PlatformAddressFFI. Rust-side TryFrom<PlatformAddressFFI> for PlatformAddress (packages/rs-platform-wallet-ffi/src/platform_address_types.rs) accepts only address_type == 0, and validate_recipient_addresses in top_up.rs re-enforces P2PKH-only. Unsupported types pass the synchronous preflight, the Task is detached, the FFI is entered, and only then a generic invalid-parameter error surfaces — defeating the documented purpose of the preflight (synchronous fail-fast before paying for Task detach + handle marshaling, per the doc comment at lines 590-593). Also load-bearing for the new recipientPlatformAddressType audit field: a non-zero addressType bypassing this guard taints what the back-fill stamps onto the consumed lock row. Mirror the Rust invariant next to the existing hash.count == 20 guard.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Duplicate funding recipients silently collapse in FFI BTreeMap decode
packages/rs-platform-wallet-ffi/src/platform_addresses/top_up.rs (line 233)
Verified at 08c4443 (line 246 confirmed: address_map.insert(addr, balance); discards the Option). At the C ABI trust boundary, duplicate PlatformAddress entries silently overwrite. Two consequences: (a) the funded output set the Rust orchestrator executes diverges from the ordered [TopUpRecipient] array Swift sized the fee strategy and the single-nil remainder against — after dedup Rust may see a smaller recipient set with a different remainder target than the fee strategy was computed against; (b) if the duplicate row flips has_balance, the lone None remainder slot can shift, redirecting fee remainder credits to a different address than the caller computed against, with no typed error crossing the boundary. The legacy fund_from_asset_lock.rs FFI was removed in commit c7828f8, so decode_funding_addresses is now the SOLE FFI decode path for address-fund recipients — the canonical place to enforce the input-shape invariant. Reject duplicates with ErrorInvalidParameter so the input shape the caller passes is the input shape the orchestrator executes.
if addresses_count == 0 {
return Ok(BTreeMap::new());
}
let mut address_map = BTreeMap::new();
for entry in std::slice::from_raw_parts(addresses, addresses_count) {
let addr = PlatformAddress::try_from(entry.address).map_err(|e| {
PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
format!("invalid platform address: {e}"),
)
})?;
let balance = if entry.has_balance {
Some(entry.balance)
} else {
None
};
if address_map.insert(addr, balance).is_some() {
return Err(PlatformWalletFFIResult::err(
PlatformWalletFFIResultCode::ErrorInvalidParameter,
"duplicate platform address in top-up recipient list".to_string(),
));
}
}
Ok(address_map)
suggestion: Successful address top-up never calls persister.store on the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 283)
Verified at 08c4443 (lines 283-326 confirmed; orchestrator goes write_address_balances_changeset → consume_asset_lock → Ok(cs) with no persister.store call). After a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount in memory and returns PlatformAddressChangeSet, but the orchestrator never invokes self.persister.store(cs.clone().into()). Sister mutating flows persist explicitly: transfer.rs and sync.rs both call self.persister.store(cs.into()). The IS→CL queue_asset_lock_changeset (line 242) DOES persist the asset-lock side, but the address-balance changeset returned to the FFI never lands in the persister. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local SwiftData store doesn't know the new credit balance — affecting auto-input selection and any UI reading balances from the persisted store. The new consume_asset_lock warn-vs-error classification (lines 305-322) implicitly assumes the changeset has already been durably recorded; it hasn't. The doc comment at line 260 promises 'Step 4: bookkeeping + cleanup' but the bookkeeping half stops at in-memory state.
suggestion: Progress view cannot disambiguate concurrent address top-ups on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressTopUpProgressView.swift (line 58)
Verified at 08c4443 (file renamed from AddressFundingProgressView.swift; @query scope unchanged). _activeLocks is @Query-scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc; currentStep, step3WasSkipped, step4WasSkipped, and broadcastSubStep all key off activeLocks.first. AddressTopUpController is keyed by (walletId, platformAccountIndex, recipientHash) precisely to allow concurrent top-ups, but PersistentAssetLock carries no recipient-hash discriminator during the in-flight phase (recipientPlatformAddressHash is stamped only post-Consumed per the new destination convention doc). With two concurrent address top-ups on one wallet, both progress views observe the same lock set and whichever row most recently sorted to first drives both steppers — Top-Up A's UI can advance to Step 3 because Top-Up B's lock just transitioned to Broadcast. PendingPlatformTopUpsList explicitly invites concurrent flows to coexist, so this matters more, not less. Fix shape: plumb the asset-lock outpoint into the controller after broadcast and scope the query by it.
suggestion: PendingPlatformTopUpsList anti-join is a no-op — same lock can render twice and be resumed while in flight
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift (line 45)
Verified at 08c4443 (file renamed from PendingPlatformFundingsList.swift). orphans = resumableLocks(excludingControllerOutpoints: Set(inFlight.compactMap { _ in nil })) — every compactMap entry returns nil, so the exclusion set is always empty by construction (the inline comment at lines 48-57 acknowledges this and notes the plumbing is here for a future tweak). The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but an in-flight controller whose asset lock has just broadcast sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens TopUpPlatformAddressView in resume mode against the same outpoint; the user can pick a different recipient, and the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch the duplicate because the second submission keys differently. Bounded (Platform deterministically rejects the duplicate ST) but the user pays a full FFI round-trip on a guaranteed-to-fail submission. Same fix path as the progress-view finding: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.
suggestion: Post-condition catches empty address_infos but not partial — missing recipients silently dropped while the asset lock is consumed
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 265)
Verified at 08c4443 (lines 273-281 confirmed: only address_infos.is_empty() is rejected). The post-condition hardens the all-empty case: returns AddressSync and the asset lock stays Resumable. The partial case is not symmetric. expected_recipient_count = addresses.len() is captured (line 273) but only used in the error message, never compared as an invariant. Downstream in write_address_balances_changeset, each requested recipient missing from address_infos emits tracing::error! and continues, but the orchestrator then proceeds to consume_asset_lock(out_point) (line 304) unconditionally on the success path. A proof returning AddressInfo for N-1 of N requested recipients therefore consumes the asset lock with one recipient invisibly dropped from the persisted changeset. Threat model: a buggy or compromised DAPI intermediary can stitch a proof that attests for fewer recipients than requested — the client refuses to recurse but still consumes its own asset lock, so the user pays on-chain locked Dash while one recipient's audit row is marked Consumed with no balance changeset. The only signal is a tracing::error! (no metric, no structured error). Either extend the post-condition to address_infos.len() == expected_recipient_count and refuse to consume on shortfall, or surface a structured PlatformWalletError::ProofIncomplete variant the caller can react to.
let expected_recipient_count = addresses.len();
let matched_recipient_count = address_infos
.iter()
.filter(|(addr, _)| addresses.contains_key(addr))
.count();
if matched_recipient_count != expected_recipient_count {
return Err(PlatformWalletError::AddressSync(format!(
"Address-funding ST succeeded but the proof returned {} of {} expected recipient(s); refusing to consume the asset lock on a partial proof result",
matched_recipient_count,
expected_recipient_count
)));
}
suggestion: top_up orchestrator has no direct tests; new branches landed unpinned
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 129)
Verified at 08c4443 (method renamed fund_addresses_with_funding → top_up; no orchestration tests added). submit_with_cl_height_retry has an explicit regression-pinning test in wallet/asset_lock/orchestration.rs, but the orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. Behaviorally distinct branches that remain unpinned: (a) the IS-rejection path advancing tracked status InstantSendLocked → ChainLocked via advance_asset_lock_status + queue_asset_lock_changeset (lines 234-242) BEFORE the second submit_with_cl_height_retry — load-bearing for the truthful-status-on-double-failure invariant Swift's resume UI consumes; (b) the empty-address_infos post-condition returning PlatformWalletError::AddressSync(...) (lines 274-281) instead of consuming the asset lock — this error variant crosses the FFI boundary to Swift; (c) the split warn/error logging on consume_asset_lock failure (lines 304-322); (d) the per-recipient skip-and-continue path in write_address_balances_changeset. A targeted #[tokio::test(start_paused = true)] set would pin the orchestration shape. PR description still marks end-to-end testnet validation as pending.
suggestion: Snapshot-delta back-fill 'multi-match → refuse to stamp' leaves both rows permanently unrecipiented
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift (line 708)
Verified at 08c4443 (file renamed from FundPlatformAddressView.swift; default branch of switch newRows.count unchanged). When two address-top-up flows resolve Consumed in the same .onChange(of: controller?.phase) window, both controllers see newRows.count > 1 and both refuse to stamp. The accompanying doc comment claims controllers 'will retry on their next phase tick … in practice one will land first and the other re-runs against the now-smaller delta.' But .completed is the terminal phase of AddressTopUpController.phase; the controller does not keep ticking after it lands there, and .onChange(of: controller?.phase) fires once per transition into .completed. If both controllers' .completed transitions land on the same MainActor turn (or close enough that both .onChange closures run before either back-fill saves), both back-fills see the same multi-element delta, both refuse, and neither row is stamped — permanently. UX-only (storage explorer shows '—' on both rows), not a correctness defect. Fix: in the multi-match branch, stamp the row matching the current controller's outpoint if the controller has one — same plumbing the progress-view and orphan-list findings want.
nitpick: decode_funding_addresses still relies on outer callers to null-check addresses when addresses_count > 0
packages/rs-platform-wallet-ffi/src/platform_addresses/top_up.rs (line 222)
Verified at 08c4443 (lines 222-249). The short-circuit at lines 230-232 returns early when addresses_count == 0 and the SAFETY doc scopes the non-null obligation to that case, closing the dominant UB hole (from_raw_parts(NULL, 0)). When addresses_count > 0 (line 234) the helper still calls std::slice::from_raw_parts(addresses, addresses_count) without verifying !addresses.is_null(). Both current FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. The helper is pub(super) and sits on a C ABI trust boundary; a future intra-module caller that omits check_ptr! for the non-zero case would reintroduce UB the optimizer is free to compile into a memory-safety bug on attacker-controlled FFI input. The deletion of fund_from_asset_lock.rs makes this helper the sole FFI decode path for address-fund recipients, so any future caller forgetting check_ptr! regresses the entire recipient-decode surface. Sibling decoders in platform_address_types.rs defensively null-check internally — aligning here removes a sharp edge from a security boundary at zero runtime cost.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
CI's `cargo fmt --all -- --check` failed on PR #3671 — the post- rename `mod top_up` ordering in the `platform_addresses/mod.rs` files and a `let Some(...) = account...` chain wrap in `top_up.rs` both needed rustfmt normalization. Pure formatting; no behavior change. Also caught a stale `AddressFundingController` reference in a doc-comment on `PlatformAddressWallet::top_up` that the earlier mass rename missed (was inside a `///` doc-comment so it didn't break compilation, but it would have confused anyone following the "Cancellation" section to the Swift side). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift (1)
47-58:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOutpoint anti-join is a no-op, so duplicate resume can race in-flight funding.
At Line 47-58, the excluded set is always empty (
compactMapalways returnsnil), so orphaned locks are never de-duplicated against active controllers. This can surface a Resume action for a lock that is already being submitted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift` around lines 47 - 58, The excluded set passed to resumableLocks is always empty because the compactMap closure returns nil; change the closure to return each controller's outpoint (e.g., inFlight.compactMap { $0.outpoint }) so the Set contains actual outpoints to anti-join against resumableLocks and prevent showing Resume for locks already handled by an in-flight controller (ensure the property name matches the controller type, and keep the Set(...) wrapper).packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpCoordinator.swift (1)
101-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRetry after failure does not re-arm retention cleanup.
On Line 101 and Line 104-110, retrying from
.failedcallssubmitbut skipsscheduleRetentionSweep. Since the original sweep returns on failure at Line 169-170, a later successful retry is never auto-purged.💡 Proposed fix
case .idle, .failed: // Legitimate restart paths. existing.submit(body: body) - // No retention sweep here — the slot is sticky on - // .failed (we want the user to see + dismiss the - // error) and a duplicate sweep on retry would just - // spawn a second 30s poll Task against the same - // controller. Sweep was already scheduled when the - // controller was first created. + // Re-arm sweep on retry. The original sweep exits on + // `.failed`, so successful retries need a new watcher. + scheduleRetentionSweep(key: key, controller: existing) return existingAlso applies to: 169-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpCoordinator.swift` around lines 101 - 110, The retry path currently calls existing.submit(body:) for cases .idle and .failed but intentionally skips calling scheduleRetentionSweep(), so when the original retention sweep aborted on failure the retried successful submission is never auto-purged; modify the .failed retry path to re-arm the retention cleanup by invoking scheduleRetentionSweep() after calling existing.submit(body:) (same for any other retry path that currently bypasses scheduling), ensuring the controller's retention sweep is scheduled whenever a submit transitions a controller back to an active/queued state so a later success will be auto-removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpCoordinator.swift`:
- Around line 101-110: The retry path currently calls existing.submit(body:) for
cases .idle and .failed but intentionally skips calling
scheduleRetentionSweep(), so when the original retention sweep aborted on
failure the retried successful submission is never auto-purged; modify the
.failed retry path to re-arm the retention cleanup by invoking
scheduleRetentionSweep() after calling existing.submit(body:) (same for any
other retry path that currently bypasses scheduling), ensuring the controller's
retention sweep is scheduled whenever a submit transitions a controller back to
an active/queued state so a later success will be auto-removed.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift`:
- Around line 47-58: The excluded set passed to resumableLocks is always empty
because the compactMap closure returns nil; change the closure to return each
controller's outpoint (e.g., inFlight.compactMap { $0.outpoint }) so the Set
contains actual outpoints to anti-join against resumableLocks and prevent
showing Resume for locks already handled by an in-flight controller (ensure the
property name matches the controller type, and keep the Set(...) wrapper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 286ac9f1-1beb-47ba-8648-f4aa2f60b2ef
📒 Files selected for processing (27)
packages/rs-platform-wallet-ffi/src/identity_registration_funded_with_signer.rspackages/rs-platform-wallet-ffi/src/platform_address_types.rspackages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet-ffi/src/platform_addresses/mod.rspackages/rs-platform-wallet-ffi/src/platform_addresses/top_up.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/identity/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/identity/network/registration.rspackages/rs-platform-wallet/src/wallet/identity/network/top_up.rspackages/rs-platform-wallet/src/wallet/identity/types/funding.rspackages/rs-platform-wallet/src/wallet/identity/types/mod.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/platform_addresses/mod.rspackages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAssetLock.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpController.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/AddressTopUpCoordinator.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Services/PlatformWalletManager+AddressTopUpCoordinator.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressTopUpProgressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift
💤 Files with no reviewable changes (6)
- packages/rs-platform-wallet/src/wallet/identity/types/funding.rs
- packages/rs-platform-wallet/src/wallet/identity/types/mod.rs
- packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
- packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs
- packages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rs
- packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-platform-wallet-ffi/src/platform_address_types.rs
- packages/rs-platform-wallet/src/wallet/identity/mod.rs
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental cumulative review at 3ea2f3a. The delta from 08c4443 is two small fixes: FFI decode_funding_addresses now rejects duplicate platform addresses with ErrorInvalidParameter (resolves prior finding #2), and TopUpPlatformAddressView filters Core funding accounts by confirmed balance with a matching canSubmit gate. No new defects introduced. Of 9 prior findings: 1 FIXED, 8 STILL VALID and carried forward (the persister.store gap, partial-proof post-condition, Swift preflight missing addressType check, concurrent-flow disambiguation across three files, missing orchestrator tests, and the latent null-check gap in the now-sole FFI decode helper). No blockers.
🟡 7 suggestion(s) | 💬 1 nitpick(s)
8 additional finding(s)
suggestion: Swift preflight accepts TopUpRecipient.addressType values Rust rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
Verified at 3ea2f3a (preflight body unchanged at 594-613). topUpFromCorePreflight validates non-emptiness, the single-nil remainder invariant, and hash.count == 20, but never inspects addressType. Both entry points marshal TopUpRecipient.addressType unchanged into PlatformAddressFFI. Rust-side TryFrom<PlatformAddressFFI> for PlatformAddress accepts only address_type == 0, and validate_recipient_addresses re-enforces P2PKH-only. Unsupported types pass the synchronous preflight, the Task is detached, the FFI is entered, and only then a generic invalid-parameter error surfaces — defeating the documented purpose of the preflight (synchronous fail-fast before paying for Task detach + handle marshaling, per the doc comment at 590-593). Also load-bearing for the recipientPlatformAddressType audit field: a non-zero addressType bypassing this guard would taint what the back-fill stamps onto the consumed lock row. Mirror the Rust invariant next to the existing hash.count == 20 guard.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Successful address top-up never calls persister.store on the changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 283)
Verified at 3ea2f3a (grep for persister.store in this file returns no matches; orchestrator goes write_address_balances_changeset → consume_asset_lock → Ok(cs)). After a successful submit, write_address_balances_changeset mutates ManagedPlatformAccount in memory and returns PlatformAddressChangeSet, but the orchestrator never invokes self.persister.store(cs.clone().into()). Sister mutating flows persist explicitly: transfer.rs and sync.rs both call self.persister.store(cs.into()). The IS→CL queue_asset_lock_changeset does persist the asset-lock side, but the address-balance changeset returned to Swift via PlatformAddressChangeSetFFI never lands in the persister. A crash between consume_asset_lock and the next BLAST sync leaves a window where Platform has accepted the top-up but the local SwiftData store doesn't know the new credit balance — affecting auto-input selection and any UI reading balances from the persisted store. On restart, initialize_from_persisted can repopulate balances from stale rows while the asset lock is already marked consumed — exactly the cross-store skew other wallet mutators explicitly avoid. The consume_asset_lock warn-vs-error classification at 304-322 implicitly assumes the changeset has already been durably recorded; it hasn't.
suggestion: Post-condition catches empty address_infos but not partial — missing recipients silently dropped while the asset lock is consumed
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 265)
Verified at 3ea2f3a (lines 273-281 confirmed: only address_infos.is_empty() is rejected). The post-condition hardens the all-empty case: returns AddressSync and the asset lock stays Resumable. The partial case is not symmetric. expected_recipient_count = addresses.len() is captured at line 273 but only used in the error message, never compared as an invariant. Downstream in write_address_balances_changeset, each requested recipient missing from address_infos emits tracing::error! and continues, but the orchestrator then proceeds to consume_asset_lock(out_point) at 304 unconditionally on the success path. A proof returning AddressInfo for N-1 of N requested recipients therefore consumes the asset lock with one recipient invisibly dropped from both the persisted changeset and the FFI-returned PlatformAddressChangeSetFFI. Threat model: a buggy or compromised DAPI intermediary can stitch a proof attesting for fewer recipients than requested — the client refuses to recurse but still consumes its own asset lock, so the user pays on-chain locked Dash while one recipient's audit row is marked Consumed with no balance changeset. Only signal is a tracing::error! (no metric, no structured error). Either extend the post-condition to compare matched-recipient count against expected_recipient_count and refuse to consume on shortfall, or surface a structured PlatformWalletError::ProofIncomplete variant the FFI caller can react to.
let expected_recipient_count = addresses.len();
let matched_recipient_count = address_infos
.iter()
.filter(|(addr, _)| addresses.contains_key(addr))
.count();
if matched_recipient_count != expected_recipient_count {
return Err(PlatformWalletError::AddressSync(format!(
"Address-funding ST succeeded but the proof returned {} of {} expected recipient(s); refusing to consume the asset lock on a partial proof result",
matched_recipient_count,
expected_recipient_count
)));
}
suggestion: Progress view cannot disambiguate concurrent address top-ups on one wallet
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/AddressTopUpProgressView.swift (line 58)
Verified at 3ea2f3a (@query scope at 67-72 unchanged). _activeLocks is @Query-scoped only by walletId == walletId && fundingTypeRaw == 4, sorted by updatedAt desc; currentStep, step3WasSkipped, step4WasSkipped, and broadcastSubStep all key off activeLocks.first. AddressTopUpController is keyed by (walletId, platformAccountIndex, recipientHash) precisely to allow concurrent top-ups, but PersistentAssetLock carries no recipient-hash discriminator during the in-flight phase (recipientPlatformAddressHash is stamped only post-Consumed per the destination convention doc). With two concurrent address top-ups on one wallet, both progress views observe the same lock set and whichever row most recently sorted to first drives both steppers — Top-Up A's UI can advance to Step 3 because Top-Up B's lock just transitioned to Broadcast. PendingPlatformTopUpsList explicitly invites concurrent flows to coexist, so this matters more, not less. Fix shape: plumb the asset-lock outpoint into the controller after broadcast and scope the query by it.
suggestion: PendingPlatformTopUpsList anti-join is a no-op — same lock can render twice and be resumed while in flight
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift (line 45)
Verified at 3ea2f3a (lines 45-58 unchanged; the compactMap closure body unconditionally returns nil). orphans = resumableLocks(excludingControllerOutpoints: Set(inFlight.compactMap { _ in nil })) — the exclusion set is always empty by construction (the inline comment acknowledges this and notes the plumbing is here for a future tweak). The SwiftData status filter (statusRaw ∈ [1,3]) excludes Consumed locks, but an in-flight controller whose asset lock has just broadcast sits in status 2/3 long enough to render in the orphan list while its driving controller is concurrently in the in-flight list. Tapping Resume opens TopUpPlatformAddressView in resume mode against the same outpoint; the user can pick a different recipient, and the coordinator's single-flight gate (keyed on (walletId, platformAccountIndex, recipientHash)) doesn't catch the duplicate because the second submission keys differently. Bounded (Platform deterministically rejects the duplicate ST) but the user pays a full FFI round-trip on a guaranteed-to-fail submission. Same fix path as the progress-view finding: have the controller expose its asset-lock outpoint once broadcast and feed it into the exclusion set.
suggestion: Snapshot-delta back-fill 'multi-match → refuse to stamp' leaves both rows permanently unrecipiented
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift (line 717)
Verified at 3ea2f3a (default branch of switch newRows.count unchanged). When two address-top-up flows resolve Consumed in the same .onChange(of: controller?.phase) window, both controllers see newRows.count > 1 and both refuse to stamp. The accompanying doc comment claims controllers will retry on their next phase tick, but .completed is the terminal phase of AddressTopUpController.phase; the controller does not keep ticking after it lands there, and .onChange(of: controller?.phase) fires once per transition into .completed. If both controllers' .completed transitions land on the same MainActor turn (or close enough that both .onChange closures run before either back-fill saves), both back-fills see the same multi-element delta, both refuse, and neither row is stamped — permanently. UX-only (storage explorer shows '—' on both rows), not a correctness defect. Fix: in the multi-match branch, stamp the row matching the current controller's outpoint if the controller has one — same plumbing the progress-view and orphan-list findings want.
suggestion: top_up orchestrator has no direct tests; new branches landed unpinned
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 129)
Verified at 3ea2f3a (no tests added to top_up.rs; file has no #[cfg(test)] module). submit_with_cl_height_retry has an explicit regression-pinning test in wallet/asset_lock/orchestration.rs, but the orchestrator composing preflight, funding resolution, IS→CL timeout fallback, IS-rejection fallback, retry submission, balance writeback, and asset-lock consume has no direct test. Behaviorally distinct branches that remain unpinned and cross the FFI boundary: (a) the IS-rejection path advancing tracked status InstantSendLocked → ChainLocked via advance_asset_lock_status + queue_asset_lock_changeset BEFORE the second submit_with_cl_height_retry — load-bearing for the truthful-status-on-double-failure invariant Swift's resume UI consumes; (b) the empty-address_infos post-condition returning PlatformWalletError::AddressSync(...) instead of consuming the asset lock — this error variant crosses the FFI boundary to Swift; (c) the split warn/error logging on consume_asset_lock failure (304-322); (d) the per-recipient skip-and-continue path in write_address_balances_changeset. A targeted #[tokio::test(start_paused = true)] set would pin the orchestration shape. PR description still marks end-to-end testnet validation as pending.
nitpick: decode_funding_addresses still relies on outer callers to null-check addresses when addresses_count > 0
packages/rs-platform-wallet-ffi/src/platform_addresses/top_up.rs (line 222)
Verified at 3ea2f3a. The duplicate-collapse defect from prior finding #2 is fixed (lines 246-257 reject duplicates with ErrorInvalidParameter), but the null-pointer hardening sub-issue remains. The short-circuit at 230-232 returns early when addresses_count == 0 and the SAFETY doc scopes the non-null obligation to that case, closing the dominant UB hole (from_raw_parts(NULL, 0)). When addresses_count > 0 (line 234) the helper still calls std::slice::from_raw_parts(addresses, addresses_count) without verifying !addresses.is_null(). Both current FFI entry points gate with check_ptr!(addresses) before the call, so current call sites are sound. The helper is pub(super) and sits on a C ABI trust boundary; a future intra-module caller that omits check_ptr! for the non-zero case would reintroduce UB the optimizer is free to compile into a memory-safety bug on attacker-controlled FFI input. The deletion of fund_from_asset_lock.rs makes this helper the sole FFI decode path for address-fund recipients. Sibling decoders in platform_address_types.rs defensively null-check internally — aligning here removes a sharp edge from a security boundary at zero runtime cost.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
CI clippy ran with `-D warnings` and flagged the new trait method + two impls at 8 args > 7. The signer-pair shape is unavoidable here (sdk, asset_lock_proof, asset_lock_proof_path, fee_strategy, signer, asset_lock_signer, settings — plus self on impls). Matches the existing `broadcast_request_for_new_identity_with_signer` allow on the identity-side broadcast helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "11efb2baec8e8d352d5bb67ae1957ba41a46a5103ab0b81b3710cd9f4dddbe66"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I did not find any new defects introduced by the latest incremental delta at 723942aba862cd7dbe3a67649c0be71eb5dc135b. Five carried-forward prior findings remain valid after source verification; two earlier prior findings are still intentionally deferred per the PR notes, and one prior FFI hygiene nitpick does not apply to the current code because every internal caller already null-checks the pointer before the helper dereferences it.
🟡 5 suggestion(s)
5 additional finding(s)
suggestion: Swift preflight allows address types that the Rust top-up path rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
topUpFromCorePreflight only validates recipient count and 20-byte hashes, but both top-up entry points marshal TopUpRecipient.addressType straight into PlatformAddressFFI at lines 446-451 and 548-553. On the Rust side, TryFrom<PlatformAddressFFI> accepts only address_type == 0, and validate_recipient_addresses accepts only PlatformAddress::P2pkh. A non-zero type therefore passes the documented Swift fail-fast preflight, crosses the FFI boundary, and fails later as a Rust invalid-parameter error. That splits the boundary contract across languages and defeats the purpose of the synchronous preflight.
for r in recipients {
guard r.addressType == 0 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.addressType currently supports only 0 (P2PKH); got \(r.addressType)"
)
}
guard r.hash.count == 20 else {
throw PlatformWalletError.invalidParameter(
"TopUpRecipient.hash must be exactly 20 bytes (got \(r.hash.count))"
)
}
}
suggestion: Successful top-ups update memory but never persist the returned address changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 283)
After a successful proof-verified submit, this path updates the in-memory account via write_address_balances_changeset(...), optionally consumes the tracked asset lock, and returns Ok(cs), but it never calls self.persister.store(cs.clone().into()). Sibling mutating flows such as packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs and packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs do persist their balance changes. The Swift FFI layer only decodes the returned PlatformAddressChangeSetFFI into [UpdatedBalance]; it does not write those balances back to the Rust persister. A restart after Platform accepted the top-up but before a later sync will therefore reload stale platform-address balances from initialize_from_persisted, even though the asset lock may already be consumed.
suggestion: Top-up still consumes the asset lock even when some recipient proofs are unusable
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 273)
The new guard rejects only the completely empty proof case. After that, the function always writes balances, consumes the tracked asset lock, and returns success. The SDK already enforces that the proof contains the expected address keys, but AddressInfos still allows None values for those keys, and write_address_balances_changeset explicitly logs-and-skips None entries at lines 356-367 instead of failing. That means a proof shape the code itself describes as a contract violation can still result in a partial changeset while the funding lock is permanently consumed. The post-condition needs to reject any recipient whose proof entry is missing usable AddressInfo before cleanup runs.
suggestion: The pending/resumable anti-join never excludes in-flight locks
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift (line 45)
resumableLocks(excludingControllerOutpoints:) is called with Set(inFlight.compactMap { _ in nil }), so the exclusion set is always empty. The comment confirms controllers do not currently expose the outpoint they are driving. As written, an asset lock that is already being processed by an in-flight controller can still appear in the resumable list at the same time, and the user can open a second resume flow against the same outpoint. The coordinator only single-flights on (walletId, platformAccountIndex, recipientHash), so choosing a different recipient hash bypasses that protection even though Platform will later reject the duplicate consume.
suggestion: Concurrent successful top-ups can leave both consumed locks permanently unstamped
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift (line 717)
backfillRecipientOnConsumedLock() runs only when activeController?.phase changes to .completed at lines 137-147. In the snapshot-delta branch, any newRows.count > 1 path logs and returns without writing recipientPlatformAddressHash or recipientPlatformAddressType. When two top-ups complete in the same visibility window, both controllers can observe the same multi-match set, both refuse to stamp, and neither controller gets another phase transition that would retry the backfill. The result is a permanent metadata hole in storage even though each controller already knows its own recipient identity.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:594-613: Swift preflight allows address types that the Rust top-up path rejects
`topUpFromCorePreflight` only validates recipient count and 20-byte hashes, but both top-up entry points marshal `TopUpRecipient.addressType` straight into `PlatformAddressFFI` at lines 446-451 and 548-553. On the Rust side, `TryFrom<PlatformAddressFFI>` accepts only `address_type == 0`, and `validate_recipient_addresses` accepts only `PlatformAddress::P2pkh`. A non-zero type therefore passes the documented Swift fail-fast preflight, crosses the FFI boundary, and fails later as a Rust invalid-parameter error. That splits the boundary contract across languages and defeats the purpose of the synchronous preflight.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs`:283-326: Successful top-ups update memory but never persist the returned address changeset
After a successful proof-verified submit, this path updates the in-memory account via `write_address_balances_changeset(...)`, optionally consumes the tracked asset lock, and returns `Ok(cs)`, but it never calls `self.persister.store(cs.clone().into())`. Sibling mutating flows such as [`packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs`](/Users/claw/.openclaw/workspace/worktrees/review-platform-3671/packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:156) and [`packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs`](/Users/claw/.openclaw/workspace/worktrees/review-platform-3671/packages/rs-platform-wallet/src/wallet/platform_addresses/sync.rs:80) do persist their balance changes. The Swift FFI layer only decodes the returned `PlatformAddressChangeSetFFI` into `[UpdatedBalance]`; it does not write those balances back to the Rust persister. A restart after Platform accepted the top-up but before a later sync will therefore reload stale platform-address balances from `initialize_from_persisted`, even though the asset lock may already be consumed.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs`:273-285: Top-up still consumes the asset lock even when some recipient proofs are unusable
The new guard rejects only the completely empty proof case. After that, the function always writes balances, consumes the tracked asset lock, and returns success. The SDK already enforces that the proof contains the expected address keys, but `AddressInfos` still allows `None` values for those keys, and `write_address_balances_changeset` explicitly logs-and-skips `None` entries at lines 356-367 instead of failing. That means a proof shape the code itself describes as a contract violation can still result in a partial changeset while the funding lock is permanently consumed. The post-condition needs to reject any recipient whose proof entry is missing usable `AddressInfo` before cleanup runs.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/PendingPlatformTopUpsList.swift`:45-58: The pending/resumable anti-join never excludes in-flight locks
`resumableLocks(excludingControllerOutpoints:)` is called with `Set(inFlight.compactMap { _ in nil })`, so the exclusion set is always empty. The comment confirms controllers do not currently expose the outpoint they are driving. As written, an asset lock that is already being processed by an in-flight controller can still appear in the resumable list at the same time, and the user can open a second resume flow against the same outpoint. The coordinator only single-flights on `(walletId, platformAccountIndex, recipientHash)`, so choosing a different recipient hash bypasses that protection even though Platform will later reject the duplicate consume.
- [SUGGESTION] In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TopUpPlatformAddressView.swift`:717-739: Concurrent successful top-ups can leave both consumed locks permanently unstamped
`backfillRecipientOnConsumedLock()` runs only when `activeController?.phase` changes to `.completed` at lines 137-147. In the snapshot-delta branch, any `newRows.count > 1` path logs and returns without writing `recipientPlatformAddressHash` or `recipientPlatformAddressType`. When two top-ups complete in the same visibility window, both controllers can observe the same multi-match set, both refuse to stamp, and neither controller gets another phase transition that would retry the backfill. The result is a permanent metadata hole in storage even though each controller already knows its own recipient identity.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at 974142d confirms the latest delta is only a #[allow(clippy::too_many_arguments)] annotation with no behavioral change. Of the five prior findings: prior-1 (Swift preflight gap on addressType), prior-2 (top-up never persists the returned PlatformAddressChangeSet), and prior-3 (asset lock consumed even when proof returns None entries for some recipients) are STILL VALID against current code and carried forward. Prior-4 (resumable anti-join always empty) and prior-5 (multi-match backfill skip) are also still present at the code level but the author has explicitly documented both as intentional acceptances in adjacent comments — moved to resolved_findings as INTENTIONALLY DEFERRED. No new findings introduced by the 723942a..974142d delta.
Note: Inline posting failed (command failed (1): gh api /repos/dashpay/platform/pulls/3671/reviews --method POST --input -
STDOUT:
{"message":"Unprocessable Entity","errors":["Variable $threads of type [DraftPullRequestReviewThread] was provided invalid value for 0.commitId (Field is not defined on DraftPullRequestReviewThread)), so I posted the same verified findings as a top-level review body.
🟡 3 suggestion(s)
3 additional finding(s)
suggestion: Asset lock is consumed even when the proof returns None for some recipients
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 273)
The post-submit guard at 273-281 only rejects the wholly-empty address_infos.is_empty() case, but write_address_balances_changeset at 351-368 also treats per-entry None as a protocol-contract violation, logs it, and skips the balance write while continuing the loop. Control then falls through to consume_asset_lock at line 304 unconditionally, so a multi-recipient top-up whose proof attests some recipients but not others still burns the asset lock while silently dropping the unfunded recipients, and the row is marked terminal Consumed so the Resume path will not pick it up. The function's own comment at 265-272 already calls this case a 'DAPI / proof-verifier contract violation, NOT a successful zero-credit funding' — the empty-map check should be extended to reject any-None as well so the lock stays in non-Consumed status and the operator can re-drive. Today's only in-tree caller is single-recipient so this is latent, but the function signature accepts arbitrary multi-recipient maps and asset locks represent non-refundable L1 funds.
suggestion: Successful top-up updates in-memory account but never persists the returned changeset
packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs (line 283)
After a verified submit, this path calls write_address_balances_changeset(...) to mutate the in-memory ManagedPlatformAccount, optionally consumes the tracked asset lock, and returns Ok(cs) — but it never sends cs through self.persister.store(cs.clone().into()). Sibling mutating address flows do persist their returned changeset (platform_addresses/transfer.rs:155-158 and platform_addresses/sync.rs:79-84). As coded, the credited balance lives only in memory until the next BLAST sync writes a row over the top; on app restart/reload before that, initialize_from_persisted will seed account.address_credit_balance from the stale pre-top-up persisted row and overwrite the freshly credited in-memory state — causing later auto_select_inputs to under-budget inputs and producing protocol-level rejections until the next sync. Either persist the changeset here matching the sibling pattern, or document the explicit guarantee that another sync call will run before any reload.
suggestion: Swift preflight does not reject addressType values the Rust FFI rejects
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (line 594)
topUpFromCorePreflight validates only recipients.isEmpty, the noneCount == 1 invariant, and r.hash.count == 20. It never inspects TopUpRecipient.addressType, even though the field is documented as 0 = P2PKH, 1 = P2SH. Mirrors the Rust-side PlatformAddress discriminant. Both topUpFromCore and resumeTopUpFromAssetLock marshal r.addressType straight into PlatformAddressFFI(address_type: r.addressType, ...). On the Rust side, impl TryFrom<PlatformAddressFFI> for PlatformAddress in packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45 accepts only address_type == 0 and returns Err("Unsupported address type") for anything else (including the 1 = P2SH discriminant the From direction will emit). A caller following the documented Swift API and passing addressType: 1 clears the preflight, pays for the Task detach and signer pinning, and only then receives a generic FFI error — defeating the preflight's stated purpose of producing a synchronous, type-specific Swift error. Either tighten the preflight to require r.addressType == 0 (lower-risk, matches the rest of the funding pipeline which assumes P2PKH for the asset-lock recipient hash) or extend the Rust TryFrom to accept 1 => P2sh.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs`:273-326: Asset lock is consumed even when the proof returns None for some recipients
The post-submit guard at 273-281 only rejects the wholly-empty `address_infos.is_empty()` case, but `write_address_balances_changeset` at 351-368 also treats per-entry `None` as a protocol-contract violation, logs it, and skips the balance write while continuing the loop. Control then falls through to `consume_asset_lock` at line 304 unconditionally, so a multi-recipient top-up whose proof attests some recipients but not others still burns the asset lock while silently dropping the unfunded recipients, and the row is marked terminal Consumed so the Resume path will not pick it up. The function's own comment at 265-272 already calls this case a 'DAPI / proof-verifier contract violation, NOT a successful zero-credit funding' — the empty-map check should be extended to reject any-None as well so the lock stays in non-Consumed status and the operator can re-drive. Today's only in-tree caller is single-recipient so this is latent, but the function signature accepts arbitrary multi-recipient maps and asset locks represent non-refundable L1 funds.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/platform_addresses/top_up.rs`:283-326: Successful top-up updates in-memory account but never persists the returned changeset
After a verified submit, this path calls `write_address_balances_changeset(...)` to mutate the in-memory `ManagedPlatformAccount`, optionally consumes the tracked asset lock, and returns `Ok(cs)` — but it never sends `cs` through `self.persister.store(cs.clone().into())`. Sibling mutating address flows do persist their returned changeset (`platform_addresses/transfer.rs:155-158` and `platform_addresses/sync.rs:79-84`). As coded, the credited balance lives only in memory until the next BLAST sync writes a row over the top; on app restart/reload before that, `initialize_from_persisted` will seed `account.address_credit_balance` from the stale pre-top-up persisted row and overwrite the freshly credited in-memory state — causing later `auto_select_inputs` to under-budget inputs and producing protocol-level rejections until the next sync. Either persist the changeset here matching the sibling pattern, or document the explicit guarantee that another sync call will run before any reload.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:594-613: Swift preflight does not reject addressType values the Rust FFI rejects
`topUpFromCorePreflight` validates only `recipients.isEmpty`, the `noneCount == 1` invariant, and `r.hash.count == 20`. It never inspects `TopUpRecipient.addressType`, even though the field is documented as `0 = P2PKH, 1 = P2SH. Mirrors the Rust-side PlatformAddress discriminant.` Both `topUpFromCore` and `resumeTopUpFromAssetLock` marshal `r.addressType` straight into `PlatformAddressFFI(address_type: r.addressType, ...)`. On the Rust side, `impl TryFrom<PlatformAddressFFI> for PlatformAddress` in `packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45` accepts only `address_type == 0` and returns `Err("Unsupported address type")` for anything else (including the `1 = P2SH` discriminant the `From` direction will emit). A caller following the documented Swift API and passing `addressType: 1` clears the preflight, pays for the Task detach and signer pinning, and only then receives a generic FFI error — defeating the preflight's stated purpose of producing a synchronous, type-specific Swift error. Either tighten the preflight to require `r.addressType == 0` (lower-risk, matches the rest of the funding pipeline which assumes P2PKH for the asset-lock recipient hash) or extend the Rust `TryFrom` to accept `1 => P2sh`.
Issue being fixed or feature implemented
Follow-up to #3634 (Core-funded identity registration via asset-lock proofs). That PR shipped the identity-create side of the asset-lock-funded pipeline end-to-end through Swift. This PR ships the platform-address funding side: build asset-lock tx from a Core (SPV) balance → wait for IS-lock (or fall back to ChainLock) → submit an
AddressFundingFromAssetLockTransition→ consume the lock on success. No asset-lock private key crosses the FFI boundary; both Core-side derivation and the outer ST signature route through an externalkey_wallet::signer::Signer.AddressFundingFromAssetLockTransitionalready existed at the DPP/SDK/wallet layers but only on a raw-private-key path. Nothing was wired end-to-end through the wallet orchestrator, FFI, Swift SDK, or app UI.What was done?
End-to-end Core→Platform address top-up, plus a UX layer matching identity-registration's parity, plus reviewer-driven hardening.
Pipeline. New
PlatformAddressWallet::top_up<S, AS>covers build → broadcast → wait-IS-or-CL → submit-with-CL-retry → IS→CL-fallback → consume. The CL-height retry helper and IS-timeout-aware funding resolver were lifted out ofwallet/identity/network/registration.rsinto a sharedwallet/asset_lock/orchestration.rsso identity-register, identity-top-up, and address-top-up all share one source of truth for retry budgets and timeouts.IdentityFundingenum becameAssetLockFundingin the same move.Signer pair. New
try_from_asset_lock_with_signers<S, AS>onAddressFundingFromAssetLockTransitionmirrors the identity-create signers pattern from #3634.try_from_asset_lock_with_signerwas renamed_with_signer_and_private_keyfor parity with that rename (4 in-tree callers updated). NewTopUpAddress::top_up_with_signerson the SDK trait threadssettings.user_fee_increaseend-to-end — load-bearing for the CL-height retry path (silent drop would let retries hash identically and hit Tenderdash's 24h invalid-tx cache).FFI + Swift.
platform_address_wallet_top_up_signer+_resume_top_up_with_existing_asset_lock_signermirror the identity-side FFI shape. Swift wrappers (topUpFromCore,resumeTopUpFromAssetLock) follow the swift-sdk CLAUDE.md "thin marshaller" rule — internalMnemonicResolver()pinned viawithExtendedLifetime((signer, coreSigner)).Example app UX. Top-up entry via a
+button on the Platform Balance row. The flow drives a 5-step live progress view (Building → Broadcasting → Waiting for IS → Waiting for CL → Funding platform address) backed by anAddressTopUpController+AddressTopUpCoordinatormirroring the identity-side single-flight + retention-sweep pattern. A "Pending Platform Top Ups" card on the wallet-detail screen surfaces both in-flight controllers and orphanedPersistentAssetLockrows for crash-recovery resume. Storage Explorer shows the recipient platform address on consumed top-up locks (back-filled by Swift on.completedsince Rust doesn't know the recipient — it's chosen at ST-submit time).Reviewer findings. Six review agents ran in parallel after the initial pipeline landed. Notable items addressed: silent-zero / index-0 fallbacks in
write_address_balances_changesetnowtracing::error!and skip; recipient-hash back-fill uses a pre-submit outpoint snapshot (deterministic, not racy newest-updatedAt); recipient type plumbed through the controller (was hardcoded P2PKH); bech32m HRP picker now reads the wallet's network (was hardcodedtdash); standalone progress view gained a Dismiss button on.failed; FFIdecode_funding_addressesrejects duplicates; Core funding-account picker filters toconfirmed > 0andcanSubmitgates onbalance >= amount. Latency budget + cancellation contract + retry scope documented at the orchestration layer.How Has This Been Tested?
cargo check --workspaceclean;cargo clippy --all-targets -- -D warningsclean.cargo test -p platform-wallet --lib— 124/124 green (includes the liftedsubmit_with_cl_height_retryregression pin).cargo test -p dpp --lib state_transition::state_transitions::address_funds— 90/90 green.Breaking Changes
AddressFundingFromAssetLockTransitionMethodsV0::try_from_asset_lock_with_signer→try_from_asset_lock_with_signer_and_private_key. Newtry_from_asset_lock_with_signers<S, AS>sibling for the external-signer shape.IdentityFundingenum moved toplatform_wallet::AssetLockFunding. The old alias was removed — external consumers should follow the rename.PlatformAddressWallet::newgains anasset_locks: Arc<AssetLockManager<SpvBroadcaster>>parameter. Only in-tree caller isPlatformWallet::new.PersistentAssetLockgains two optional SwiftData fields (recipientPlatformAddressHash,recipientPlatformAddressType) — lightweight migration; existing rows readnil.platform_address_wallet_fund_from_asset_lockFFI and its wallet counterpart removed. The orchestratedtop_up_signerentry point covers the same use case viaAssetLockFunding::FromExistingAssetLock.Checklist: